Mailing-List: contact dev-help@apex.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@apex.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAMqituOghY-AwibmKkP1iRJ6p_gFVQZXAWHBEDf7m7zHMO4G6g@mail.gmail.com>
References: 
 <CAKaBFBm75M7D4EfesG29q_j4wFAd3eDAUOoG3p2v82B53jSXCA@mail.gmail.com>
	<CADS+zCv7FMOdVkFJgFFuZA59jFXz1YsCg=eXCLrcWuPUicB4Qg@mail.gmail.com>
	<CAOi1F6XAWWJH2GYyQGaHxrqkk=1XCZ8iLvZdpojfm_nNAU4R+A@mail.gmail.com>
	<CAMqituOOaujSqu8p88ZtcVNf1DwtLvOrgtikeJs1Rb7bdLXcFg@mail.gmail.com>
	<CAKaBFBm6MYkdssBB02UmEprR5ghrdxqzawKJLGEa1ivR+vynCQ@mail.gmail.com>
	<CAMqituOghY-AwibmKkP1iRJ6p_gFVQZXAWHBEDf7m7zHMO4G6g@mail.gmail.com>
Date: Thu, 29 Oct 2015 16:38:05 -0700
Message-ID: 
 <CAKaBFB=ucQopPXeyXhGNA3h-WF5stq8ngTEEVFCNOOZBdDHViw@mail.gmail.com>
Subject: Re: [CodeStyle] Maximum length of a line
From: Chandni Singh <chandni@datatorrent.com>
To: dev@apex.incubator.apache.org
Content-Type: multipart/alternative; boundary=001a11443fe47f5912052346cdf9

--001a11443fe47f5912052346cdf9
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi David,

Wrapping indentation of '2' comes from the existing code in Apex core. I
think in your example you can just add a blank line after while statement
and that avoids confusion.


while (baz() ||
  foo() ||
  bar()) {

  foo();
  bar();
}

Chandni

On Thu, Oct 29, 2015 at 4:14 PM, David Yan <david@datatorrent.com> wrote:

> Sorry I might have missed the prior discussion about continuation
> indentation.
> I think continuation indentation should not be 2 because it would align
> with the block indentation and can cause confusion.
> For example:
>
> while (baz() ||
>   foo() ||
>   bar()) {
>   foo();
>   bar();
> }
>
> vs
>
> while (baz() ||
>     foo() ||
>     bar()) {
>   foo();
>   bar();
> }
>
>
>
> On Thu, Oct 29, 2015 at 4:10 PM, Chandni Singh <chandni@datatorrent.com>
> wrote:
>
> > The convention that we adopted for wrapping is
> > public void test(int a,
> >     int b,
> >     int c)
> > Continuation indentation is set to 2 more (4 from the start of line)
> extra
> > space.
> >
> > I don't think prematurely wrapping helps readability. In Ram's example
> when
> > there are comments then yes it helps but I am talking about scenarios
> where
> > there aren't comments.
> > I think just seeing a lot of empty space and scrolling more doesn't
> include
> > readability.
> >
> > Chandni
> >
> > On Thu, Oct 29, 2015 at 3:58 PM, David Yan <david@datatorrent.com>
> wrote:
> >
> > > I'm +1 on 120 since I'm using a tall screen to code now. :)
> > >
> > > Did we discuss how the code should be indented for lines that break
> > because
> > > of the limit?
> > > For example,
> > >
> > > public void test(int a,
> > >     int b,
> > >     int c)
> > >
> > > or
> > >
> > > public void test(int a,
> > >                  int b,
> > >                  int c)
> > >
> > > The first one uses a fixed width of 4 extra spaces to indent the
> > > continuations, and you won't need to add spaces to the continuation
> lines
> > > if you rename "test" to be "foo" or "somelongfunctionname".
> > >
> > > But the second one looks better to the eye.
> > >
> > > Also, should the operator be at the end of the line or at the beginni=
ng
> > of
> > > the line?
> > >
> > > For example:
> > >
> > > if (a ||
> > >     b &&
> > >     c)
> > >
> > > or
> > >
> > > if (a
> > >   || b
> > >   && c)
> > >
> > > David
> > >
> > > On Thu, Oct 29, 2015 at 3:57 PM, Siyuan Hua <siyuan@datatorrent.com>
> > > wrote:
> > >
> > > > +1 for 160 for 4k monitors
> > > >
> > > > On Thu, Oct 29, 2015 at 3:55 PM, Timothy Farkas <tim@datatorrent.co=
m
> >
> > > > wrote:
> > > >
> > > > > +1 for 120
> > > > >
> > > > > On Thu, Oct 29, 2015 at 2:28 PM, Chandni Singh <
> > > chandni@datatorrent.com>
> > > > > wrote:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > As part of the discussion below it was brought out that we need
> to
> > > > > improve
> > > > > > the readability of our code and therefore impose a maximum line
> > > limit.
> > > > > >
> > > > > > If you have a preference for maximum length of a line in the
> code,
> > > then
> > > > > > please voice it now.
> > > > > >
> > > > > > So far Brennon, Ram, Vlad prefer 120.
> > > > > > I am with Thomas on 160.
> > > > > >
> > > > > > This rule can be enforced by checkstyle. However I have also
> > noticed
> > > > > > prematurely wrapping lines, for example,
> > > > > >
> > > > > > public void tes(int t,
> > > > > >                          int x,
> > > > > >                           int z);
> > > > > >
> > > > > > This shouldn't be allowed as well. However checkstyle cannot
> > enforce
> > > > > this.
> > > > > > So it's the responsibility of developers and reviewers to corre=
ct
> > > this.
> > > > > >
> > > > > > Thanks,
> > > > > > Chandni
> > > > > >
> > > > > > On Thu, Oct 29, 2015 at 10:55 AM, Chandni Singh <
> > > > chandni@datatorrent.com
> > > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi All,
> > > > > > >
> > > > > > > Do we have a consensus on line length?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Chandni
> > > > > > >
> > > > > > > On Tue, Oct 20, 2015 at 4:38 PM, Vlad Rozov <
> > > v.rozov@datatorrent.com
> > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > >> It depends, some screen are tall, not wide :-) . I tried 160
> on
> > my
> > > > > tall
> > > > > > >> screen and it looks OK, but I use smaller font size. My
> > > > recommendation
> > > > > > is
> > > > > > >> to keep it to 120.
> > > > > > >>
> > > > > > >> Thank you,
> > > > > > >>
> > > > > > >> Vlad
> > > > > > >>
> > > > > > >>
> > > > > > >> On 10/20/15 15:38, Thomas Weise wrote:
> > > > > > >>
> > > > > > >>> How about being a bit more generous with line length: 160?
> > > Screens
> > > > > are
> > > > > > >>> pretty wide nowadays..
> > > > > > >>>
> > > > > > >>> +1 for documentation as part of PR review. Documentation
> should
> > > not
> > > > > > only
> > > > > > >>> be
> > > > > > >>> present but also helpful. Maybe we can enforce presence for
> > newly
> > > > > added
> > > > > > >>> classes by tracking count of existing violations?
> > > > > > >>>
> > > > > > >>> On Tue, Oct 20, 2015 at 2:38 PM, Munagala Ramanath <
> > > > > > ram@datatorrent.com>
> > > > > > >>> wrote:
> > > > > > >>>
> > > > > > >>> Yes, 120 line length max is good.
> > > > > > >>>>
> > > > > > >>>> Yes, agree we need to find some way to enforce javadocs.
> > > > > > >>>>
> > > > > > >>>> Ram
> > > > > > >>>>
> > > > > > >>>> On Tue, Oct 20, 2015 at 11:45 AM, York, Brennon <
> > > > > > >>>> Brennon.York@capitalone.com
> > > > > > >>>>
> > > > > > >>>>> wrote:
> > > > > > >>>>> For 1 and 2 I=C2=B9ve made a JIRA to track
> > > > > > >>>>> (https://malhar.atlassian.net/browse/APEX-204).
> > > > > > >>>>>
> > > > > > >>>>> For 3) I definitely agree we need line wraps. Understand
> that
> > > > > > everyone
> > > > > > >>>>>
> > > > > > >>>> has
> > > > > > >>>>
> > > > > > >>>>> different length monitors, but, as a community, we should
> > agree
> > > > on
> > > > > a
> > > > > > >>>>> standard moving forward as this becomes a community-owned
> > > > project.
> > > > > > How
> > > > > > >>>>> does 120 sound?
> > > > > > >>>>>
> > > > > > >>>>> For 4) if we want to start treating Apex as an Apache
> project
> > > > owned
> > > > > > by
> > > > > > >>>>>
> > > > > > >>>> the
> > > > > > >>>>
> > > > > > >>>>> community that uses it we need to start working *for* the
> > > > > community /
> > > > > > >>>>> developers who are going to contribute to it, not merely
> > > continue
> > > > > on
> > > > > > as
> > > > > > >>>>>
> > > > > > >>>> if
> > > > > > >>>>
> > > > > > >>>>> the people currently working on it will be the only prima=
ry
> > > > > drivers.
> > > > > > >>>>> That
> > > > > > >>>>> won=C2=B9t engender growth or community engagement. If no=
thing
> > else
> > > we
> > > > > > >>>>> should
> > > > > > >>>>> be prepared to open our doors to new ideas and
> functionality
> > to
> > > > the
> > > > > > >>>>> project, not make it more difficult through obfuscated
> code.
> > It
> > > > > > hasn=C2=B9t
> > > > > > >>>>> been done to this point and that=C2=B9s fine, but moving
> forward I
> > > > think
> > > > > > we
> > > > > > >>>>> should take a concerted look and take this as an
> opportunity
> > to
> > > > > clean
> > > > > > >>>>> it
> > > > > > >>>>> up / document it. It will only get harder as the project
> > gains
> > > > > > >>>>> momentum.
> > > > > > >>>>> And, if this causes failures, that=C2=B9s a problem for u=
s to
> > admit,
> > > > > > accept,
> > > > > > >>>>> and fix.
> > > > > > >>>>>
> > > > > > >>>>> On 10/20/15, 10:19 AM, "Chandni Singh" <
> > > chandni@datatorrent.com>
> > > > > > >>>>> wrote:
> > > > > > >>>>>
> > > > > > >>>>> 1) This is a bug and will fix this
> > > > > > >>>>>>
> > > > > > >>>>>> 2) Another bug and will fix this
> > > > > > >>>>>>
> > > > > > >>>>>> 3) We don't have a line limit because everyone uses
> > different
> > > > > length
> > > > > > >>>>>> monitor and some prefer a much longer line. However I
> think
> > we
> > > > > need
> > > > > > to
> > > > > > >>>>>>
> > > > > > >>>>> at
> > > > > > >>>>
> > > > > > >>>>> least have a minimum length limit and only beyond this a
> line
> > > > > should
> > > > > > be
> > > > > > >>>>>> wrapped.
> > > > > > >>>>>>
> > > > > > >>>>>> 4) Earlier javadocs were strictly added to api and commo=
n
> > > > classes.
> > > > > > >>>>>> There
> > > > > > >>>>>> are hardly any for engine, bufferserver modules. Adding
> this
> > > > will
> > > > > > mean
> > > > > > >>>>>> much higher number of pre-existing failures. I am not mu=
ch
> > in
> > > > > favor
> > > > > > of
> > > > > > >>>>>> this.
> > > > > > >>>>>>
> > > > > > >>>>>> As far as the lineage is concerned, these were mostly
> taken
> > > from
> > > > > > >>>>>> google-checks and modified for the style we adopted. Als=
o
> > > > referred
> > > > > > to
> > > > > > >>>>>> sun_checks and picked a few from there which we needed.
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>> On Tue, Oct 20, 2015 at 8:42 AM, Ganelin, Ilya
> > > > > > >>>>>> <Ilya.Ganelin@capitalone.com>
> > > > > > >>>>>> wrote:
> > > > > > >>>>>>
> > > > > > >>>>>> All - there are some issues I=C2=B9ve already run into w=
ith the
> > > > > > >>>>>>> CodeStyle/CheckStyle settings. I suggest we start a JIR=
A
> to
> > > > track
> > > > > > >>>>>>>
> > > > > > >>>>>> these
> > > > > > >>>>
> > > > > > >>>>> unless you have a preferred approach.
> > > > > > >>>>>>>
> > > > > > >>>>>>> 1) CheckStyle dictates that chained method calls be on
> > > > different
> > > > > > >>>>>>> lines
> > > > > > >>>>>>> but
> > > > > > >>>>>>> also dictates that a space may not precede a period. Th=
e
> > > below
> > > > is
> > > > > > >>>>>>> thus
> > > > > > >>>>>>> invalid:
> > > > > > >>>>>>>      Foo.bar
> > > > > > >>>>>>>           .cat
> > > > > > >>>>>>> 2) Continuation Indent is set to 4 in CheckStyle but se=
t
> > to 2
> > > > by
> > > > > > >>>>>>>
> > > > > > >>>>>> default
> > > > > > >>>>
> > > > > > >>>>> in CodeStyle
> > > > > > >>>>>>> 3) We should really enforce line limits (for the sake o=
f
> > > > > > readability)
> > > > > > >>>>>>> and
> > > > > > >>>>>>> should therefore amend the wrapping behavior of methods=
.
> > > > However,
> > > > > > >>>>>>> this
> > > > > > >>>>>>> will
> > > > > > >>>>>>> require updating CheckStyle as well.
> > > > > > >>>>>>> 4) We should enforce JavaDocs
> > > > > > >>>>>>>
> > > > > > >>>>>>> As an aside, could someone possibly speak to the lineag=
e
> of
> > > the
> > > > > > >>>>>>> CheckStyle
> > > > > > >>>>>>> and CodeStyle settings that we=C2=B9re presently using =
inside
> > > Apex?
> > > > > Did
> > > > > > >>>>>>>
> > > > > > >>>>>> these
> > > > > > >>>>
> > > > > > >>>>> come from published settings (e.g. Google) or are these a=
ll
> > > > > in-house?
> > > > > > >>>>>>>
> > > > > > >>>>>>> Appreciate any input, thanks!
> > > > > > >>>>>>> _______________________________________________________=
_
> > > > > > >>>>>>>
> > > > > > >>>>>>> The information contained in this e-mail is confidentia=
l
> > > and/or
> > > > > > >>>>>>> proprietary to Capital One and/or its affiliates and ma=
y
> > only
> > > > be
> > > > > > used
> > > > > > >>>>>>> solely in performance of work or services for Capital
> One.
> > > The
> > > > > > >>>>>>> information
> > > > > > >>>>>>> transmitted herewith is intended only for use by the
> > > individual
> > > > > or
> > > > > > >>>>>>> entity
> > > > > > >>>>>>> to which it is addressed. If the reader of this message
> is
> > > not
> > > > > the
> > > > > > >>>>>>> intended
> > > > > > >>>>>>> recipient, you are hereby notified that any review,
> > > > > retransmission,
> > > > > > >>>>>>> dissemination, distribution, copying or other use of, o=
r
> > > taking
> > > > > of
> > > > > > >>>>>>> any
> > > > > > >>>>>>> action in reliance upon this information is strictly
> > > > prohibited.
> > > > > If
> > > > > > >>>>>>>
> > > > > > >>>>>> you
> > > > > > >>>>
> > > > > > >>>>> have received this communication in error, please contact
> the
> > > > > sender
> > > > > > >>>>>>>
> > > > > > >>>>>> and
> > > > > > >>>>
> > > > > > >>>>> delete the material from your computer.
> > > > > > >>>>>>>
> > > > > > >>>>>>> _______________________________________________________=
_
> > > > > > >>>>>>>
> > > > > > >>>>>>> The information contained in this e-mail is confidentia=
l
> > > and/or
> > > > > > >>>>>>> proprietary to Capital One and/or its affiliates and ma=
y
> > only
> > > > be
> > > > > > used
> > > > > > >>>>>>> solely in performance of work or services for Capital
> One.
> > > The
> > > > > > >>>>>>> information
> > > > > > >>>>>>> transmitted herewith is intended only for use by the
> > > individual
> > > > > or
> > > > > > >>>>>>> entity
> > > > > > >>>>>>> to which it is addressed. If the reader of this message
> is
> > > not
> > > > > the
> > > > > > >>>>>>> intended
> > > > > > >>>>>>> recipient, you are hereby notified that any review,
> > > > > retransmission,
> > > > > > >>>>>>> dissemination, distribution, copying or other use of, o=
r
> > > taking
> > > > > of
> > > > > > >>>>>>> any
> > > > > > >>>>>>> action in reliance upon this information is strictly
> > > > prohibited.
> > > > > If
> > > > > > >>>>>>>
> > > > > > >>>>>> you
> > > > > > >>>>
> > > > > > >>>>> have received this communication in error, please contact
> the
> > > > > sender
> > > > > > >>>>>>>
> > > > > > >>>>>> and
> > > > > > >>>>
> > > > > > >>>>> delete the material from your computer.
> > > > > > >>>>>>>
> > > > > > >>>>>>>
> > > > > > >>>>>>> _______________________________________________________=
_
> > > > > > >>>>>
> > > > > > >>>>> The information contained in this e-mail is confidential
> > and/or
> > > > > > >>>>> proprietary to Capital One and/or its affiliates and may
> only
> > > be
> > > > > used
> > > > > > >>>>> solely in performance of work or services for Capital One=
.
> > The
> > > > > > >>>>>
> > > > > > >>>> information
> > > > > > >>>>
> > > > > > >>>>> transmitted herewith is intended only for use by the
> > individual
> > > > or
> > > > > > >>>>> entity
> > > > > > >>>>> to which it is addressed. If the reader of this message i=
s
> > not
> > > > the
> > > > > > >>>>>
> > > > > > >>>> intended
> > > > > > >>>>
> > > > > > >>>>> recipient, you are hereby notified that any review,
> > > > retransmission,
> > > > > > >>>>> dissemination, distribution, copying or other use of, or
> > taking
> > > > of
> > > > > > any
> > > > > > >>>>> action in reliance upon this information is strictly
> > > prohibited.
> > > > If
> > > > > > you
> > > > > > >>>>> have received this communication in error, please contact
> the
> > > > > sender
> > > > > > >>>>> and
> > > > > > >>>>> delete the material from your computer.
> > > > > > >>>>>
> > > > > > >>>>> ________________________________________________________
> > > > > > >>>>>
> > > > > > >>>>> The information contained in this e-mail is confidential
> > and/or
> > > > > > >>>>> proprietary to Capital One and/or its affiliates and may
> only
> > > be
> > > > > used
> > > > > > >>>>> solely in performance of work or services for Capital One=
.
> > The
> > > > > > >>>>>
> > > > > > >>>> information
> > > > > > >>>>
> > > > > > >>>>> transmitted herewith is intended only for use by the
> > individual
> > > > or
> > > > > > >>>>> entity
> > > > > > >>>>> to which it is addressed. If the reader of this message i=
s
> > not
> > > > the
> > > > > > >>>>>
> > > > > > >>>> intended
> > > > > > >>>>
> > > > > > >>>>> recipient, you are hereby notified that any review,
> > > > retransmission,
> > > > > > >>>>> dissemination, distribution, copying or other use of, or
> > taking
> > > > of
> > > > > > any
> > > > > > >>>>> action in reliance upon this information is strictly
> > > prohibited.
> > > > If
> > > > > > you
> > > > > > >>>>> have received this communication in error, please contact
> the
> > > > > sender
> > > > > > >>>>> and
> > > > > > >>>>> delete the material from your computer.
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

--001a11443fe47f5912052346cdf9--