mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mu Li <muli....@gmail.com>
Subject Re: Increase indentation limit from 100 to 120 characters
Date Mon, 08 Jan 2018 19:12:37 GMT
I think this code should be refactored instead of increasing the indent.
Puting the test codes within 7 nested for loops is hard to read. Maybe we
can split it into multiple functions, or create some templates, or try
"using mxnet::op::BatchNormV1Prop"

In general, I feel 80-column is not a big problem, at least it forces me to
write short sentences. The C++ codes use Google style, if Google can fit 1
billion lines of codes into this style, then we may want to think about how
to refactor our long sentences instead of loosening the constraint.

On Mon, Jan 8, 2018 at 1:53 AM, kellen sunderland <
kellen.sunderland@gmail.com> wrote:

> It's probably good to have an example to help with discussion.  Here's one
> that's been bugging us, and highlights why the current line length limit in
> C++ leads to hard-to-read code:
> https://github.com/larroy/mxnet/blob/467a79c8b9f3a75ce993302c6d0c85
> 8628cb1cdc/tests/cpp/operator/batchnorm_test.cc#L963
>
> On Sat, Jan 6, 2018 at 12:00 PM, kellen sunderland <
> kellen.sunderland@gmail.com> wrote:
>
> > Just a note that I don't think Pedro was suggesting the change for Python
> > or Scala.  How would folks feel about changing the limit for just C++?
> >
> > On Sat, Jan 6, 2018 at 6:21 AM, Tianqi Chen <tqchen@cs.washington.edu>
> > wrote:
> >
> >> An argument against such change would be the coding style standard is
> >> people already get used to it, and there is less benefit of making the
> >> change.
> >>
> >> PEP and Google C style suggest 80 chars as limit, I usually write with
> >> that
> >> in mind and try to break multiple arguments into multiple lines when
> such
> >> violation happens, and rarely sometimes have a 100 line code for code
> >> reason
> >>
> >> One potential benefit of fewer characters per line makes it easier to do
> >> split editing when you split your code into two screens (hey emacs and
> vim
> >> users)
> >>
> >> I am not in strong favor of either number of line limits but is
> >> comfortable
> >> with the current setting
> >>
> >>
> >> Tianqi
> >>
> >> On Fri, Jan 5, 2018 at 11:28 AM, Chris Olivier <cjolivier01@gmail.com>
> >> wrote:
> >>
> >> > Thank you for the excellent reply!
> >> >
> >> > On Fri, Jan 5, 2018 at 11:25 AM, Nan Zhu <zhunanmcgill@gmail.com>
> >> wrote:
> >> >
> >> > > well....max line length as 100 is adopted in many projects (nearly
> all
> >> > > projects I have been involved or used or looked at,
> >> > > spark/flink/bahir/atlas, etc. companies which using scala
> intensively
> >> > also
> >> > > sets it to 100 (e.g. netflix, you can check their atlas project))
> >> > >
> >> > > one of the reasons is that all these projects are all following
> >> > > https://github.com/databricks/scala-style-guide which was published
> >> in
> >> > the
> >> > > early days of when scala is becoming popular
> >> > >
> >> > > and the behind reason might be that considering the language
> >> > > characteristics of scala, a shorter line limit would be make it more
> >> > > readable, (http://docs.scala-lang.org/style/indentation.html#line-
> >> > wrapping
> >> > > ,
> >> > > the official guide even says 80 as the limit)
> >> > >
> >> > > Also note that, scala-packages has a scala-style plugin regulating
> >> coding
> >> > > style which does not apply limits for certain cases, e.g. import,
> and
> >> the
> >> > > developer can turn off style checking if you are doing something
> >> special
> >> > >
> >> > >
> >> > > BTW, considering monitor-relevant concern,
> >> > http://scalameta.org/scalafmt/
> >> > > tells that 100 is good enough even for a 30'' wide monitor
> >> > >
> >> > >
> >> > >
> >> > >
> >> > > On Fri, Jan 5, 2018 at 11:10 AM, Chris Olivier <
> cjolivier01@gmail.com
> >> >
> >> > > wrote:
> >> > >
> >> > > > Why -1?
> >> > > >
> >> > > > On Fri, Jan 5, 2018 at 11:03 AM, Nan Zhu <zhunanmcgill@gmail.com>
> >> > wrote:
> >> > > >
> >> > > > > -1 for scala part
> >> > > > >
> >> > > > > On Fri, Jan 5, 2018 at 9:48 AM, Marco de Abreu <
> >> > > > > marco.g.abreu@googlemail.com
> >> > > > > > wrote:
> >> > > > >
> >> > > > > > +1
> >> > > > > >
> >> > > > > > Am 05.01.2018 5:49 nachm. schrieb "Chris Olivier" <
> >> > > > cjolivier01@gmail.com
> >> > > > > >:
> >> > > > > >
> >> > > > > > +1
> >> > > > > >
> >> > > > > > On Fri, Jan 5, 2018 at 8:00 AM, Pedro Larroy <
> >> > > > > pedro.larroy.lists@gmail.com
> >> > > > > > >
> >> > > > > > wrote:
> >> > > > > >
> >> > > > > > > Hi
> >> > > > > > >
> >> > > > > > > Can we please increase the indent limit from 100
to 120? I
> >> find
> >> > 100
> >> > > > > > > too low for current standards and today's monitors.
Default
> >> CLion
> >> > > > line
> >> > > > > > > limit is also 120.
> >> > > > > > >
> >> > > > > > > I'm having to split some long templates and I
wish we had a
> >> > longer
> >> > > > line
> >> > > > > > > limit.
> >> > > > > > >
> >> > > > > > > Thanks a lot.
> >> > > > > > >
> >> > > > > > > Pedro
> >> > > > > > >
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message