Could we use Aljoscha’s proposed checkstyle [0] as a baseline for discussion?
It does not define a line length and we could bid down from there.
Chesnay not only reviewed the changes but summarized the checkstyle [1]:
It enforces the following rules:
• every file has to end with a new-line
• no trailing whitespace anywhere
• every public/protected method/class must have a javadoc
• imports have to be sorted alphabetically
• static imports must come first
• whitespace after commas, semicolons and casts
• whitespace around operators, if, for, etc.
• left curly brace ({) must not be at the start of the line
• else/catch/finally must be on the same line as the right curly brace
(})
• static final variables must be upper case
• non-static variables must not be upper case
[0]
https://github.com/aljoscha/flink/blob/b60e1b8885beafe46eb7ec2552aa71cfe175aa95/tools/maven/strict-checkstyle.xml
[1] https://github.com/apache/flink/pull/3567#issuecomment-287764234
<https://github.com/apache/flink/pull/3567#issuecomment-287764234>
> On Apr 4, 2017, at 11:55 PM, Jinkui Shi <[email protected]> wrote:
>
> It’s glad to restart check style discuss.
> I’m agree with Stephan’s strategy.
>
> For the ongoing partial pull request and companies’ fork, need to be rewrite
> following new style rule. That must be finished themselves.
>
> We can discuss the check style rule detail one by one(checkstyle.xml,
> scalastyle-config.xml). The rules should be accept by most of developers.
>
> I provide one rule to discuss:
> - maxLineLength: 120(java and scala)
>
> [1]https://issues.apache.org/jira/browse/FLINK-4519
> <https://issues.apache.org/jira/browse/FLINK-4519>
>> 在 2017年3月6日，下午7:42，Stephan Ewen <[email protected]> 写道：
>>
>> 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 <[email protected]>
>> 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/
>>>
>>> On Mon, Feb 27, 2017 at 11:54 AM, Stavros Kontopoulos <
>>> [email protected]> 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
>>>>
>>>> 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 <[email protected]> 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 <[email protected]
>>>>
>>>>> 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 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 <[email protected]>
>>>> 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 <
>>>>>>> [email protected]> 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 <
>>>>> [email protected]
>>>>>>>
>>>>>>>> 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 <
>>> [email protected]>
>>>>>>> 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 <
>>>>>>>>>> [email protected]> 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 <[email protected]>:
>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske <
>>>>>>> [email protected]
>>>>>>>>>
>>>>>>>>>>> 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 ;-)).
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
