apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chandni Singh <chan...@datatorrent.com>
Subject Re: [CodeStyle] Maximum length of a line
Date Thu, 29 Oct 2015 23:38:05 GMT
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 beginning
> > 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.com
> >
> > > > 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 correct
> > > 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¹ve 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 primary
> > > > > drivers.
> > > > > > >>>>> That
> > > > > > >>>>> won¹t engender growth or community engagement.
If nothing
> > 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¹t
> > > > > > >>>>> been done to this point and that¹s 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¹s a
problem for us 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 common
> > > > classes.
> > > > > > >>>>>> There
> > > > > > >>>>>> are hardly any for engine, bufferserver
modules. Adding
> this
> > > > will
> > > > > > mean
> > > > > > >>>>>> much higher number of pre-existing
failures. I am not much
> > 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. Also
> > > > 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¹ve already
run into with the
> > > > > > >>>>>>> CodeStyle/CheckStyle settings.
I suggest we start a JIRA
> 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. The
> > > below
> > > > is
> > > > > > >>>>>>> thus
> > > > > > >>>>>>> invalid:
> > > > > > >>>>>>>      Foo.bar
> > > > > > >>>>>>>           .cat
> > > > > > >>>>>>> 2) Continuation Indent is set to
4 in CheckStyle but set
> > to 2
> > > > by
> > > > > > >>>>>>>
> > > > > > >>>>>> default
> > > > > > >>>>
> > > > > > >>>>> in CodeStyle
> > > > > > >>>>>>> 3) We should really enforce line
limits (for the sake of
> > > > > > 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 lineage
> of
> > > the
> > > > > > >>>>>>> CheckStyle
> > > > > > >>>>>>> and CodeStyle settings that we¹re
presently using inside
> > > Apex?
> > > > > Did
> > > > > > >>>>>>>
> > > > > > >>>>>> these
> > > > > > >>>>
> > > > > > >>>>> come from published settings (e.g. Google)
or are these all
> > > > > in-house?
> > > > > > >>>>>>>
> > > > > > >>>>>>> Appreciate any input, thanks!
> > > > > > >>>>>>> ________________________________________________________
> > > > > > >>>>>>>
> > > > > > >>>>>>> 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
> is
> > > 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
> is
> > > 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 is
> > 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 is
> > 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.
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

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