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 Tue, 18 Apr 2017 18:39:57 GMT
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