So to sum up all the comments so far we have two alternatives. We either: 1) introduce unified checkstyle (with enforcing) and corresponding code style, both based on some established ones like google code style for java [1] <https://github.com/google/google-java-format> and scalastyle for scala [2] <http://www.scalastyle.org/> . We would introduce it module by module for a longer period of time or 2) leave it as it is, and end this discussion for a longer (possibly infinite :) ) period of time
Not sure how we should proceed with the decision on it. Is it possible to do some voting or so? 2017-02-27 20:54 GMT+01:00 Stavros Kontopoulos <st.kontopou...@gmail.com>: > +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 <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> > > 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 <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> 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 > > > > > > > > > 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> > > > > 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> 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>: > > > > > > > > > > > > > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > > > > 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 ;-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
