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
> >> > > > > > > > >
> >> > > > > > > >
> >> > > > > > >
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>
