apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vlad Rozov <v.ro...@datatorrent.com>
Subject Re: continuation indentation
Date Fri, 30 Oct 2015 00:48:50 GMT
I believe checkstyle currently does not enforce any indentation for the 
while() construct and unfortunately the following is considered valid:

   public boolean foo()
   {
     while (baz() ||
                         foo() ||
                  bar()) {
       foo();
       bar();
     }
     return true;
   }

I am not sure that there is consistency in a way how current alignment 
is done for function declaration or invocation.

To avoid block issue putting "{" on a new line will help, but we already 
agreed on a different style for blocks where "{" is on the same line.

Thank you,

Vlad


On 10/29/15 16:46, David Yan wrote:
> I think most of the existing code in Apex tries to align with the operator
> in the original line, instead of a fixed 2 extra spaces, in the
> continuation lines.
>
> Adding a blank line after while statement is not an enforced rule, and we
> should take this opportunity to get this right.
>
> David
>
>
> On Thu, Oct 29, 2015 at 4:38 PM, Chandni Singh <chandni@datatorrent.com>
> wrote:
>
>> 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