Hi All,
I suggest we also conclude this discussion now.
Breaking the line of too long statements (line longness is yet to be fully
defined) to improve code readability in case of
- Long function argument lists (declaration or call): void func(type1
arg1, type2 arg2, ...)
- Long sequence of chained calls:
list.stream().map(...).reduce(...).collect(...)...
Rules:
- Break the list of arguments/calls if the line exceeds limit or earlier
if you believe that the breaking would improve the code readability
- If you break the line then each argument/call should have a separate
line, including the first one
- Each new line argument/call should have one extra indentation relative
to the line of the parent function name or called entity
- The opening brace always stays on the line of the parent function name
- The closing brace of the function argument list and the possible
thrown exception list always stay on the line of the last argument
- The dot of a chained call is always on the line of that chained call
proceeding the call at the beginning
Examples of breaking:
- Function arguments
*public **void func(*
* int arg1,*
* int arg2,*
* ...)** throws E1, E2, E3 {*
* ...*
*}*
- Chained method calls:
*values*
* .stream()*
* .map(*...*)*
* .collect(...);*
I suggest we spawn separate discussion threads (can do as a follow-up)
about:
- the hard line length limit in Java, possibly to confirm it also for
Scala (cc @Tison)
- indentation rules for the broken list of a declared function arguments
If there are no more comments/objections/concerns, I will open a PR to
capture the discussion outcome.
Best,
Andrey
On Wed, Aug 21, 2019 at 8:57 AM Zili Chen <wander4...@gmail.com> wrote:
> Implement question: how to apply the line length rules?
>
> If we just turn on checkstyle rule "LineLength" then a huge
> effort is required to break lines those break the rule. If
> we use an auto-formatter here then it possibly break line
> "just at the position" awfully.
>
> Is it possible we require only to fit the rule on the fly
> a pull request touch files?
>
> Best,
> tison.
>
>
> Yu Li <car...@gmail.com> 于2019年8月20日周二 下午5:22写道：
>
> > I second Stephan's summarize, and to be more explicit, +1 on:
> > - Set a hard line length limit
> > - Allow arguments on the same line if below length limit
> > - With consistent argument breaking when that length is exceeded
> > - Developers can break before that if they feel it helps with readability
> >
> > FWIW, hbase project also sets the line length limit to 100 [1]
> (personally
> > I don't have any tendency on this, so JFYI).
> >
> > [1]
> >
> >
> https://github.com/apache/hbase/blob/a59f7d4ffc27ea23b9822c3c26d6aeb76ccdf9aa/hbase-checkstyle/src/main/resources/hbase/checkstyle.xml#L128
> >
> > Best Regards,
> > Yu
> >
> >
> > On Mon, 19 Aug 2019 at 18:22, Stephan Ewen <se...@apache.org> wrote:
> >
> > > I personally prefer not to break lines with few parameters.
> > > It just feels unnecessarily clumsy to parse the breaks if there are
> only
> > > two or three arguments with short names.
> > >
> > > So +1
> > > - for a hard line length limit
> > > - allowing arguments on the same line if below that limit
> > > - with consistent argument breaking when that length is exceeded
> > > - developers can break before that if they feel it helps with
> > > readability.
> > >
> > > This should be similar to what we have, except for enforcing the line
> > > length limit.
> > >
> > > I think our Java guide originally suggested 120 characters line length,
> > but
> > > we can reduce that to 100 if a majority argues for that, but it is
> > separate
> > > discussion.
> > > We uses shorter lines in Scala (100 chars) because Scala code becomes
> > > harder to read faster with long lines.
> > >
> > >
> > > On Mon, Aug 19, 2019 at 10:45 AM Andrey Zagrebin <and...@ververica.com
> >
> > > wrote:
> > >
> > > > Hi Everybody,
> > > >
> > > > Thanks for your feedback guys and sorry for not getting back to the
> > > > discussion for some time.
> > > >
> > > > @SHI Xiaogang
> > > > About breaking lines for thrown exceptions:
> > > > Indeed that would prevent growing the throw clause indefinitely.
> > > > I am a bit concerned about putting the right parenthesis and/or throw
> > > > clause on the next line
> > > > because in general we do not it and there are a lot of variations of
> > how
> > > > and what to put to the next line so it needs explicit memorising.
> > > > Also, we do not have many checked exceptions and usually avoid them.
> > > > Although I am not a big fan of many function arguments either but
> this
> > > > seems to be a bigger problem in the code base.
> > > > I would be ok to not enforce anything for exceptions atm.
> > > >
> > > > @Chesnay Schepler <ches...@apache.org>
> > > > Thanks for mentioning automatic checks.
> > > > Indeed, pointing out this kind of style issues during PR reviews is
> > very
> > > > tedious
> > > > and cannot really force it without automated tools.
> > > > I would still consider the outcome of this discussion as a soft
> > > > recommendation atm (which we also have for some other things in the
> > code
> > > > style draft).
> > > > We need more investigation about how to enforce things. I am not so
> > > > knowledgable about code style/IDE checks.
> > > > From the first glance I also do not see a simple way. If somebody has
> > > more
> > > > insight please share your experience.
> > > >
> > > > @Biao Liu <mmyy1...@gmail.com>
> > > > Line length limitation:
> > > > I do not see anything for Java, only for Scala: 100 (also enforced by
> > > build
> > > > AFAIK).
> > > > From what I heard there has been already some discussion about the
> hard
> > > > limit for the line length.
> > > > Although quite some people are in favour of it (including me) and
> seems
> > > to
> > > > be a nice limitation,
> > > > there are some practical implication about it.
> > > > Historically, Flink did not have any code style checks and huge
> chunks
> > of
> > > > code base have to be reformatted destroying the commit history.
> > > > Another thing is value for the limit. Nowadays, we have wide screens
> > and
> > > do
> > > > not often even need to scroll.
> > > > Nevertheless, we can kick off another discussion about the line
> length
> > > > limit and enforcing it.
> > > > Atm I see people to adhere to a soft recommendation of 120 line
> length
> > > for
> > > > Java because it is usually a bit more verbose comparing to Scala.
> > > >
> > > > *Question 1*:
> > > > I would be ok to always break line if there is more than one chained
> > > call.
> > > > There are a lot of places where this is only one short call I would
> not
> > > > break line in this case.
> > > > If it is too confusing I would be ok to stick to the rule to break
> > either
> > > > all or none.
> > > > Thanks for pointing out this explicitly: For a chained method calls,
> > the
> > > > new line should be started with the dot.
> > > > I think it should be also part of the rule if forced.
> > > >
> > > > *Question 2:*
> > > > The indent of new line should be 1 tab or 2 tabs? (I assume it
> matters
> > > only
> > > > for function arguments)
> > > > This is a good point which again probably deserves a separate thread.
> > > > We also had an internal discussion about it. I would be also in
> favour
> > of
> > > > resolving it into one way.
> > > > Atm we indeed have 2 ways in our code base which are again soft
> > > > recommendations.
> > > > The problem is mostly with enforcing it automatically.
> > > > The approach with extra indentation also needs IDE setup otherwise it
> > is
> > > > annoying
> > > > that after every function cut/paste, e.g. Idea changes the format to
> > one
> > > > indentation automatically and often people do not notice it.
> > > >
> > > > I suggest we still finish this discussion to a point of achieving a
> > soft
> > > > recommendation which we can also reconsider
> > > > when there are more ideas about automatically enforcing these things.
> > > >
> > > > Best,
> > > > Andrey
> > > >
> > > > On Sat, Aug 3, 2019 at 7:51 AM SHI Xiaogang <shixiaoga...@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi Chesnay,
> > > > >
> > > > > Thanks a lot for your reminder.
> > > > >
> > > > > For Intellij settings, the style i proposed can be configured as
> > below
> > > > > * Method declaration parameters: chop down if long
> > > > > * align when multiple: YES
> > > > > * new line after '(': YES
> > > > > * place ')' on new line: YES
> > > > > * Method call arguments: chop down if long
> > > > > * align when multiple: YES
> > > > > * take priority over call chain wrapping: YES
> > > > > * new line after '(': YES
> > > > > * place ')' on new line: YES
> > > > > * Throws list: chop down if long
> > > > > * align when multiline: YES
> > > > >
> > > > > As far as i know, there does not exist standard checks for the
> > > alignment
> > > > of
> > > > > method parameters or arguments. It needs further investigation to
> see
> > > > > whether we can validate these styles via customized checks.
> > > > >
> > > > >
> > > > > Biao Liu <mmyy1...@gmail.com> 于2019年8月2日周五 下午4:00写道：
> > > > >
> > > > > > Hi Andrey,
> > > > > >
> > > > > > Thank you for bringing us this discussion.
> > > > > >
> > > > > > I would like to make some details clear. Correct me if I am
> wrong.
> > > > > >
> > > > > > The guide draft [1] says the line length is limited in 100
> > > characters.
> > > > > From
> > > > > > my understanding, this discussion suggests if there is more than
> > 100
> > > > > > characters in one line (both Scala and Java), we should start a
> new
> > > > line
> > > > > > (or lines).
> > > > > >
> > > > > > *Question 1*: If a line does not exceed 100 characters, should we
> > > break
> > > > > the
> > > > > > chained calls into lines? Currently the chained calls always been
> > > > broken
> > > > > > into lines even it's not too long. Does it just a suggestion or a
> > > > > > limitation?
> > > > > > I prefer it's a limitation which must be respected. And we should
> > > > always
> > > > > > break the chained calls no matter how long the line is.
> > > > > >
> > > > > > For a chained method calls, the new line should be started with
> the
> > > > dot.
> > > > > >
> > > > > > *Question 2:* The indent of new line should be 1 tab or 2 tabs?
> > > > Currently
> > > > > > there exists these two different styles. This rule should be also
> > > > applied
> > > > > > to function arguments.
> > > > > >
> > > > > > BTW, big +1 to options from Chesnay. We should make auto-format
> > > > possible
> > > > > on
> > > > > > our project.
> > > > > >
> > > > > > 1.
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://docs.google.com/document/d/1owKfK1DwXA-w6qnx3R7t2D_o0BsFkkukGlRhvl3XXjQ/edit#
> > > > > >
> > > > > > Thanks,
> > > > > > Biao /'bɪ.aʊ/
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Fri, Aug 2, 2019 at 9:20 AM SHI Xiaogang <
> > shixiaoga...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Andrey,
> > > > > > >
> > > > > > > Thanks for bringing this. Personally, I prefer to the following
> > > style
> > > > > > which
> > > > > > > (1) puts the right parenthese in the next line
> > > > > > > (2) a new line for each exception if exceptions can not be put
> in
> > > the
> > > > > > same
> > > > > > > line
> > > > > > >
> > > > > > > That way, parentheses are aligned in a similar way to braces
> and
> > > > > > exceptions
> > > > > > > can be well aligned.
> > > > > > >
> > > > > > > *public **void func(*
> > > > > > > * int arg1,*
> > > > > > > * int arg2,*
> > > > > > > * ...
> > > > > > > *) throws E1, E2, E3 {*
> > > > > > > * ...
> > > > > > > *}*
> > > > > > >
> > > > > > > or
> > > > > > >
> > > > > > > *public **void func(*
> > > > > > > * int arg1,*
> > > > > > > * int arg2,*
> > > > > > > * ...
> > > > > > > *) throws
> > > > > > > * E1,
> > > > > > > * E2,
> > > > > > > * E3 {*
> > > > > > > * ...
> > > > > > > *}*
> > > > > > >
> > > > > > > Regards,
> > > > > > > Xiaogang
> > > > > > >
> > > > > > > Andrey Zagrebin <and...@ververica.com> 于2019年8月1日周四 下午11:19写道：
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > This is one more small suggestion for the recent thread about
> > > code
> > > > > > style
> > > > > > > > guide in Flink [1].
> > > > > > > >
> > > > > > > > We already have a note about using a new line for each
> chained
> > > call
> > > > > in
> > > > > > > > Scala, e.g. either:
> > > > > > > >
> > > > > > > > *values**.stream()**.map(...)**,collect(...);*
> > > > > > > >
> > > > > > > > or
> > > > > > > >
> > > > > > > > *values*
> > > > > > > > * .stream()*
> > > > > > > > * .map(*...*)*
> > > > > > > > * .collect(...)*
> > > > > > > >
> > > > > > > > if it would result in a too long line by keeping all chained
> > > calls
> > > > in
> > > > > > one
> > > > > > > > line.
> > > > > > > >
> > > > > > > > The suggestion is to have it for Java as well and add the
> same
> > > rule
> > > > > > for a
> > > > > > > > long list of function arguments. So it is either:
> > > > > > > >
> > > > > > > > *public void func(int arg1, int arg2, ...) throws E1, E2, E3
> {*
> > > > > > > > * ...*
> > > > > > > > *}*
> > > > > > > >
> > > > > > > > or
> > > > > > > >
> > > > > > > > *public **void func(*
> > > > > > > > * int arg1,*
> > > > > > > > * int arg2,*
> > > > > > > > * ...)** throws E1, E2, E3 {*
> > > > > > > > * ...*
> > > > > > > > *}*
> > > > > > > >
> > > > > > > > but thrown exceptions stay on the same last line.
> > > > > > > >
> > > > > > > > Please, feel free to share you thoughts.
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > Andrey
> > > > > > > >
> > > > > > > > [1]
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> http://mail-archives.apache.org/mod_mbox/flink-dev/201906.mbox/%3ced91df4b-7cab-4547-a430-85bc710fd...@apache.org%3E
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
