FYI PR: https://github.com/apache/flink-web/pull/254
On Thu, Aug 22, 2019 at 10:11 AM Andrey Zagrebin <and...@ververica.com>
wrote:
> Hi Tison,
>
> Regarding the automatic checks.
> Yes, I suggest we conclude the discussion without the automatic checks.
> As soon as we have more ideas/investigation, put into automation, we can
> activate it and/or reconsider.
> Nonetheless, I do not see any problem if we agree on this atm and make it
> part of our code style recommendations.
>
> Regarding putting the right parenthesis on the new line.
> At the moment we do not use this approach in our code base. My personal
> feeling is that it is not so often used in Java.
> Anyways I think, this goes more into direction of the second follow-up to
> discuss this separately:
>
> - indentation rules for the broken list of a declared function
> arguments (atm we usually do: one and newline before body or two idents)
>
> or maybe we should rather name it: how to separate function declaration
> and body (as you already mentioned it like this).
>
> We can change this point:
>
> - The closing brace of the function argument list and the possible
> thrown exception list always stay on the line of the last argument
>
> to
>
> - The possible thrown exception list is never broken and stays on the
> same last line
>
> Then we can also adjust it if needed after the discussion about how to
> separate function declaration and body.
>
> Best,
> Andrey
>
>
>
>
>
>
> On Thu, Aug 22, 2019 at 9:05 AM Zili Chen <wander4...@gmail.com> wrote:
>
>> One more question, what do you differ
>>
>> *public **void func(*
>> * int arg1,*
>> * int arg2,*
>> * ...)** throws E1, E2, E3 {*
>> * ...*
>> *}*
>>
>> and
>>
>> *public **void func(*
>> * int arg1,*
>> * int arg2,*
>> * ...
>> *)** throws E1, E2, E3 {*
>> * ...*
>> *}*
>>
>> I prefer the latter because parentheses are aligned in a similar way,
>> as well as the border between declaration and function body is clear.
>>
>>
>> Zili Chen <wander4...@gmail.com> 于2019年8月22日周四 上午9:53写道：
>>
>> > Thanks Andrey for driving the discussion. Just for clarification,
>> > what we conclude here are several guidelines without automatic
>> > checker/tool guard them, right?
>> >
>> > Best,
>> > tison.
>> >
>> >
>> > Andrey Zagrebin <and...@ververica.com> 于2019年8月21日周三 下午8:18写道：
>> >
>> >> 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
>> >> > > > > > > > >
>> >> > > > > > > >
>> >> > > > > > >
>> >> > > > > >
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>>
>
