BTW, the current limit is 100 loc instead of 80 loc, which is mostly enough
Tianqi On Mon, Jan 8, 2018 at 11:12 AM, Mu Li <[email protected]> wrote: > 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 < > [email protected]> 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 < > > [email protected]> 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 <[email protected]> > > > 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 <[email protected] > > > > >> wrote: > > >> > > >> > Thank you for the excellent reply! > > >> > > > >> > On Fri, Jan 5, 2018 at 11:25 AM, Nan Zhu <[email protected]> > > >> 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 < > > [email protected] > > >> > > > >> > > wrote: > > >> > > > > >> > > > Why -1? > > >> > > > > > >> > > > On Fri, Jan 5, 2018 at 11:03 AM, Nan Zhu < > [email protected]> > > >> > wrote: > > >> > > > > > >> > > > > -1 for scala part > > >> > > > > > > >> > > > > On Fri, Jan 5, 2018 at 9:48 AM, Marco de Abreu < > > >> > > > > [email protected] > > >> > > > > > wrote: > > >> > > > > > > >> > > > > > +1 > > >> > > > > > > > >> > > > > > Am 05.01.2018 5:49 nachm. schrieb "Chris Olivier" < > > >> > > > [email protected] > > >> > > > > >: > > >> > > > > > > > >> > > > > > +1 > > >> > > > > > > > >> > > > > > On Fri, Jan 5, 2018 at 8:00 AM, Pedro Larroy < > > >> > > > > [email protected] > > >> > > > > > > > > >> > > > > > 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 > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > >
