flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aljoscha Krettek <aljos...@apache.org>
Subject Re: [DISCUSS] Code style / checkstyle
Date Wed, 26 Apr 2017 14:30:38 GMT
I merged the stricter checkstyle for flink-streaming-java today. (Sans checking for right curly
braces)

> On 18. Apr 2017, at 22:17, Chesnay Schepler <chesnay@apache.org> wrote:
> 
> +1 to use earth rotation as the new standard time unit. Maybe more importantly, I'm absolutely
in favor of merging it.
> 
> On 18.04.2017 20:39, Aljoscha Krettek wrote:
>> I rebased the PR [1] on current master. Is there any strong objection against merging
this (minus the two last commits which introduce curly-brace-style checking). If not, I would
like to merge this after two earth rotations, i.e. after all the time zones have had some
time to react.
>> 
>> The complete set of checks has been listed by Chesnay (via Greg) before but the gist
of it is that we only add common-sense checks that most people should be able to agree upon
so that we avoid edit wars (especially when it comes to whitespace, import order and Javadoc
paragraph styling).
>> 
>> [1] https://github.com/apache/flink/pull/3567
>>> On 5. Apr 2017, at 23:54, Chesnay Schepler <chesnay@apache.org> wrote:
>>> 
>>> I agree to just allow both. While I definitely prefer 1) i can see why someone
might prefer 2).
>>> 
>>> Wouldn't want to delay this anymore; can't find to add this to flink-metrics
and flink-python...
>>> 
>>> On 03.04.2017 18:33, Aljoscha Krettek wrote:
>>>> I think enough people did already look at the checkstyle rules proposed in
the PR.
>>>> 
>>>> On most of the rules reaching consensus is easy so I propose to enable all
rules except those regarding placement of curly braces and control flow formatting. The two
styles in the Flink code base are:
>>>> 
>>>> 1)
>>>> if () {
>>>> } else {
>>>> }
>>>> 
>>>> try {
>>>> } catch () {
>>>> }
>>>> 
>>>> and
>>>> 
>>>> 2)
>>>> 
>>>> if () {
>>>> }
>>>> else {
>>>> }
>>>> 
>>>> try {
>>>> }
>>>> catch () {
>>>> }
>>>> 
>>>> I think it’s hard to reach consensus on these so I suggest to keep allowing
both styles.
>>>> 
>>>> Any comments very welcome! :-)
>>>> 
>>>> Best,
>>>> Aljoscha
>>>>> On 19. Mar 2017, at 17:09, Aljoscha Krettek <aljoscha@apache.org>
wrote:
>>>>> 
>>>>> I played around with this over the week end and it turns out that the
required changes in flink-streaming-java are not that big. I created a PR with a proposed
checkstyle.xml and the required code changes: https://github.com/apache/flink/pull/3567 <https://github.com/apache/flink/pull/3567>.
There’s a longer description of the style in the PR. The commits add continuously more invasive
changes so we can start with the more lightweight changes if we want to.
>>>>> 
>>>>> If we want to go forward with this I would also encourage other people
to use this for different modules and see how it turns out.
>>>>> 
>>>>> Best,
>>>>> Aljoscha
>>>>> 
>>>>>> On 18 Mar 2017, at 08:00, Aljoscha Krettek <aljoscha@apache.org
<mailto:aljoscha@apache.org>> wrote:
>>>>>> 
>>>>>> I added an issue for adding a custom checkstyle.xml for flink-streaming-java
so that we can gradually add checks: https://issues.apache.org/jira/browse/FLINK-6107 <https://issues.apache.org/jira/browse/FLINK-6107>.
I outlined the procedure in the Jira. We can use this as a pilot project and see how it goes
and then gradually also apply to other modules.
>>>>>> 
>>>>>> What do you think?
>>>>>> 
>>>>>>> On 6 Mar 2017, at 12:42, Stephan Ewen <sewen@apache.org <mailto:sewen@apache.org>>
wrote:
>>>>>>> 
>>>>>>> A singular "all reformat in one instant" will cause immense damage
to the
>>>>>>> project, in my opinion.
>>>>>>> 
>>>>>>> - There are so many pull requests that we are having a hard time
keeping
>>>>>>> up, and merging will a lot more time intensive.
>>>>>>> - I personally have many forked branches with WIP features that
will
>>>>>>> probably never go in if the branches become unmergeable. I expect
that to
>>>>>>> be true for many other committers and contributors.
>>>>>>> - Some companies have Flink forks and are rebasing patches onto
master
>>>>>>> regularly. They will be completely screwed by a full reformat.
>>>>>>> 
>>>>>>> If we do something, the only thing that really is possible is:
>>>>>>> 
>>>>>>> (1) Define a style. Ideally not too far away from Flink's style.
>>>>>>> (2) Apply it to new projects/modules
>>>>>>> (3) Coordinate carefully to pull it into existing modules, module
by
>>>>>>> module. Leaving time to adopt pull requests bit by bit, and allowing
forks
>>>>>>> to go through minor merges, rather than total conflict.
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On Wed, Mar 1, 2017 at 5:57 PM, Henry Saputra <henry.saputra@gmail.com
<mailto:henry.saputra@gmail.com>>
>>>>>>> wrote:
>>>>>>> 
>>>>>>>> It is actually Databricks Scala guide to help contributions
to Apache Spark
>>>>>>>> so not really official Spark Scala guide.
>>>>>>>> The style guide feels bit more like asking people to write
Scala in Java
>>>>>>>> mode so I am -1 to follow the style for Apache Flink Scala
if that what you
>>>>>>>> are recommending.
>>>>>>>> 
>>>>>>>> If the "unification" means ONE style guide for both Java
and Scala I would
>>>>>>>> vote -1 to it because both languages have different semantics
and styles to
>>>>>>>> make them readable and understandable.
>>>>>>>> 
>>>>>>>> We could start with improving the Scala maven style guide
to follow more
>>>>>>>> Scala official style guide [1] and add IntelliJ Idea or Eclipse
plugin
>>>>>>>> style to follow suit.
>>>>>>>> 
>>>>>>>> Java side has bit more strict style check but we could make
it tighter but
>>>>>>>> embracing more Google Java guide closely with minor exceptions
>>>>>>>> 
>>>>>>>> - Henry
>>>>>>>> 
>>>>>>>> [1] http://docs.scala-lang.org/style/ <http://docs.scala-lang.org/style/>
>>>>>>>> 
>>>>>>>> On Mon, Feb 27, 2017 at 11:54 AM, Stavros Kontopoulos <
>>>>>>>> st.kontopoulos@gmail.com <mailto:st.kontopoulos@gmail.com>>
wrote:
>>>>>>>> 
>>>>>>>>> +1 to provide and enforcing a unified code style for
both java and scala.
>>>>>>>>> Unification should apply when it makes sense like comments
though.
>>>>>>>>> 
>>>>>>>>> Eventually code base should be re-factored. I would vote
for the one at a
>>>>>>>>> time module fix apporoach.
>>>>>>>>> Style guide should be part of any PR review.
>>>>>>>>> 
>>>>>>>>> We could also have a look at the spark style guide:
>>>>>>>>> https://github.com/databricks/scala-style-guide <https://github.com/databricks/scala-style-guide>
>>>>>>>>> 
>>>>>>>>> The style code and general guidelines help keep code
more readable and
>>>>>>>> keep
>>>>>>>>> things simple
>>>>>>>>> with many contributors and different styles of code writing
+ language
>>>>>>>>> features.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On Mon, Feb 27, 2017 at 8:01 PM, Stephan Ewen <sewen@apache.org
<mailto:sewen@apache.org>> wrote:
>>>>>>>>> 
>>>>>>>>>> I agree, reformatting 90% of the code base is tough.
>>>>>>>>>> 
>>>>>>>>>> There are two main issues:
>>>>>>>>>> (1) Incompatible merges. This is hard, especially
for the folks that
>>>>>>>>> have
>>>>>>>>>> to merge the pull requests ;-)
>>>>>>>>>> 
>>>>>>>>>> (2) Author history: This is less of an issue, I think.
"git log
>>>>>>>>>> <filename>" and "git show <revision>
-- <filename>" will still work and
>>>>>>>>> one
>>>>>>>>>> may have to go one commit back to find out why something
was changed
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> What I could image is to do this incrementally. Define
the code style
>>>>>>>> in
>>>>>>>>>> "flink-parent" but do not activate it.
>>>>>>>>>> Then start with some projects (new projects, plus
some others):
>>>>>>>>>> merge/reject PRs, reformat, activate code style.
>>>>>>>>>> 
>>>>>>>>>> Piece by piece. This is realistically going to take
a long time until
>>>>>>>> it
>>>>>>>>> is
>>>>>>>>>> pulled through all components, but that's okay, I
guess.
>>>>>>>>>> 
>>>>>>>>>> Stephan
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On Mon, Feb 27, 2017 at 1:53 PM, Aljoscha Krettek
<aljoscha@apache.org <mailto:aljoscha@apache.org>
>>>>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> Just for a bit of context, this is the output
of running cloc on the
>>>>>>>>>> Flink
>>>>>>>>>>> codebase:
>>>>>>>>>>> ------------------------------------------------------------
>>>>>>>>>>> -----------------------
>>>>>>>>>>> Language                         files      
   blank        comment
>>>>>>>>>>>   code
>>>>>>>>>>> ------------------------------------------------------------
>>>>>>>>>>> -----------------------
>>>>>>>>>>> Java                              4609      
  126825         185428
>>>>>>>>>>> 519096
>>>>>>>>>>> 
>>>>>>>>>>> => 704,524 lines of code + comments/javadoc
>>>>>>>>>>> 
>>>>>>>>>>> When I apply the google style to the Flink code
base using
>>>>>>>>>>> https://github.com/google/google-java-format
<https://github.com/google/google-java-format> I get these commit
>>>>>>>>>>> statistics:
>>>>>>>>>>> 
>>>>>>>>>>> 4577 files changed, 647645 insertions(+), 622663
deletions(-)
>>>>>>>>>>> 
>>>>>>>>>>> That is, a change to the Google Code Style would
touch roughly over
>>>>>>>> 90%
>>>>>>>>>> of
>>>>>>>>>>> all code/comment lines.
>>>>>>>>>>> 
>>>>>>>>>>> I would like to have a well defined code style,
such as the Google
>>>>>>>> Code
>>>>>>>>>>> style, that has nice tooling and support but
I don't think we will
>>>>>>>> ever
>>>>>>>>>>> convince enough people to do this kind of massive
change. Even I
>>>>>>>> think
>>>>>>>>>> it's
>>>>>>>>>>> a bit crazy to change 90% of the code base in
one commit.
>>>>>>>>>>> 
>>>>>>>>>>> On Mon, 27 Feb 2017 at 11:10 Till Rohrmann <trohrmann@apache.org
<mailto:trohrmann@apache.org>>
>>>>>>>>> wrote:
>>>>>>>>>>>> No, I think that's exactly what people mean
when saying "losing the
>>>>>>>>>>> commit
>>>>>>>>>>>> history". With the reformatting you would
have to go manually
>>>>>>>> through
>>>>>>>>>> all
>>>>>>>>>>>> past commits until you find the commit which
changed a given line
>>>>>>>>>> before
>>>>>>>>>>>> the reformatting.
>>>>>>>>>>>> 
>>>>>>>>>>>> Cheers,
>>>>>>>>>>>> Till
>>>>>>>>>>>> 
>>>>>>>>>>>> On Sun, Feb 26, 2017 at 6:32 PM, Alexander
Alexandrov <
>>>>>>>>>>>> alexander.s.alexandrov@gmail.com <mailto:alexander.s.alexandrov@gmail.com>>
wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>>> Just to clarify - by "losing the commit
history" you actually
>>>>>>>> mean
>>>>>>>>>>>> "losing
>>>>>>>>>>>>> the ability to annotate each line in
a file with its last
>>>>>>>> commit",
>>>>>>>>>>> right?
>>>>>>>>>>>>> Or is there some other sense in which
something is lost after
>>>>>>>>>> applying
>>>>>>>>>>>> bulk
>>>>>>>>>>>>> re-format?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>> A.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> On Sat, Feb 25, 2017 at 7:10 AM Henry
Saputra <
>>>>>>>>>> henry.saputra@gmail.com <mailto:henry.saputra@gmail.com>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Just want to clarify what unify code
style here.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Is the intention to have IDE and
Maven plugins to have the same
>>>>>>>>>> check
>>>>>>>>>>>>> style
>>>>>>>>>>>>>> rules?
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Or are we talking about having ONE
code style for both Java and
>>>>>>>>>>> Scala?
>>>>>>>>>>>>>> - Henry
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 8:08 AM,
Greg Hogan <
>>>>>>>> code@greghogan.com <mailto:code@greghogan.com>>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>> I agree wholeheartedly with Ufuk.
We cannot reformat the
>>>>>>>>>> codebase,
>>>>>>>>>>>>> cannot
>>>>>>>>>>>>>>> pause while flushing the PR queue,
and won't find a consensus
>>>>>>>>>> code
>>>>>>>>>>>>> style.
>>>>>>>>>>>>>>> I think we can create a baseline
code style for new and
>>>>>>>>> existing
>>>>>>>>>>>>>>> contributors for which reformatting
on changed files will be
>>>>>>>>>>>> acceptable
>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>> PR reviews.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 5:01
AM, Dawid Wysakowicz <
>>>>>>>>>>>>>>> wysakowicz.dawid@gmail.com <mailto:wysakowicz.dawid@gmail.com>>
wrote:
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> The problem with code style
when it is not enforced is that
>>>>>>>>> it
>>>>>>>>>>> will
>>>>>>>>>>>>> be
>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>> matter of luck to what parts
of files / new files will it
>>>>>>>> be
>>>>>>>>>>>> applied.
>>>>>>>>>>>>>>> When
>>>>>>>>>>>>>>>> the code style is not applied
to whole file, it is pretty
>>>>>>>>> much
>>>>>>>>>>>>> useless
>>>>>>>>>>>>>>>> anyway. You would need to
manually select just the
>>>>>>>> fragments
>>>>>>>>>> one
>>>>>>>>>>> is
>>>>>>>>>>>>>>>> changing. The benefits of
having code style and enforcing
>>>>>>>> it
>>>>>>>>> I
>>>>>>>>>>> see
>>>>>>>>>>>>> are:
>>>>>>>>>>>>>>>> - being able to apply autoformatter,
which speeds up
>>>>>>>> writing
>>>>>>>>>>> code
>>>>>>>>>>>>>>>> - it would make reviewing
PRs easier as e.g. there would
>>>>>>>> be
>>>>>>>>>> line
>>>>>>>>>>>>>> length
>>>>>>>>>>>>>>>> limit applied etc. which
will make line breaking more
>>>>>>>> reader
>>>>>>>>>>>>> friendly.
>>>>>>>>>>>>>>>> Though I think if a consensus
is not reachable it would be
>>>>>>>>> good
>>>>>>>>>>> to
>>>>>>>>>>>>> once
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>>> forever decide that we don't
want a code style and
>>>>>>>>> checkstyle.
>>>>>>>>>>>>>>>> 2017-02-24 10:51 GMT+01:00
Ufuk Celebi <uce@apache.org <mailto:uce@apache.org>>:
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>> On Fri, Feb 24, 2017
at 10:46 AM, Fabian Hueske <
>>>>>>>>>>>> fhueske@gmail.com <mailto:fhueske@gmail.com>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>> I agree with Till
that encouraging a code style without
>>>>>>>>>>>> enforcing
>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>> does
>>>>>>>>>>>>>>>>>> not make a lot of
sense.
>>>>>>>>>>>>>>>>>> If we enforce it,
we need to touch all files and PRs.
>>>>>>>>>>>>>>>>> I think it makes sense
for new contributors to have a
>>>>>>>>>> starting
>>>>>>>>>>>>> point
>>>>>>>>>>>>>>>>> without enforcing anything
(I do agree that we are past
>>>>>>>> the
>>>>>>>>>>> point
>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>> reach consensus on a
style and enforcement ;-)).
>>>>>>>>>>>>>>>>> 
>> 
> 


Mime
View raw message