apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Yan <da...@datatorrent.com>
Subject Re: [CodeStyle] Maximum length of a line
Date Thu, 29 Oct 2015 23:14:52 GMT
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