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 <ches...@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 <ches...@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 <aljos...@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 <aljos...@apache.org >>>>>> <mailto:aljos...@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 <se...@apache.org >>>>>>> <mailto:se...@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.sapu...@gmail.com >>>>>>> <mailto:henry.sapu...@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.kontopou...@gmail.com <mailto:st.kontopou...@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 <se...@apache.org >>>>>>>>> <mailto:se...@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 >>>>>>>>>> <aljos...@apache.org <mailto:aljos...@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 <trohrm...@apache.org >>>>>>>>>>> <mailto:trohrm...@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.alexand...@gmail.com >>>>>>>>>>>> <mailto:alexander.s.alexand...@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.sapu...@gmail.com <mailto:henry.sapu...@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 < >>>>>>>> c...@greghogan.com <mailto:c...@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.da...@gmail.com <mailto:wysakowicz.da...@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 <u...@apache.org >>>>>>>>>>>>>>>> <mailto:u...@apache.org>>: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>>>>>>> fhue...@gmail.com <mailto:fhue...@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 ;-)). >>>>>>>>>>>>>>>>> >> >
