[DISCUSS] Code style / checkstyle
Locked
12
12
Re: [DISCUSS] Code style / checkstyle
 
|
+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 <[hidden email]> 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 <[hidden email]> > 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 <[hidden email]> 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 < > > > [hidden email]> 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 < > [hidden email] > > > > > > > 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 <[hidden email]> > > > 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 < > > > > > > [hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > > > [hidden email] > > > > > > > > > > > > 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 ;-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > |
Re: [DISCUSS] Code style / checkstyle
|
|
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 <[hidden email]>: > +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 <[hidden email]> 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 <[hidden email]> > > 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 <[hidden email]> > 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 < > > > > [hidden email]> 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 < > > [hidden email] > > > > > > > > > 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 <[hidden email]> > > > > 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 < > > > > > > > [hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > > > > [hidden email] > > > > > > > > > > > > > > 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 ;-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > |
Re: [DISCUSS] Code style / checkstyle
|
|
There was also …
- create a flink style (for example, leaving indentation as +1 tab rather than +2 spaces as in google's style) - create a flink style but optional and unenforced (but recommended for new contributions) Flink currently has a reasonably consistent code style. I expect that adopting a radically different code style module-by-module will also result in contributions with a mix of old an new styles. If we’re not willing to re-style flink-core today, under what circumstances will this change? With a punctuated refactoring there would be a singular event for developers to remember (as with the initial commits). Greg > On Feb 27, 2017, at 3:40 PM, Dawid Wysakowicz <[hidden email]> wrote: > > 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 <[hidden email]>: > >> +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 <[hidden email]> 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 <[hidden email]> >>> 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 <[hidden email]> >> 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 < >>>>> [hidden email]> 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 < >>> [hidden email] >>>>> >>>>>> 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 <[hidden email]> >>>>> 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 < >>>>>>>> [hidden email]> 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 <[hidden email]>: >>>>>>>>> >>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>> [hidden email] >>>>>>> >>>>>>>>> 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 ;-)). >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> |
Re: [DISCUSS] Code style / checkstyle
|
|
I agree with adopting a custom codestyle/checkstyle for flink, but as I
understood correctly most people agree there is no point of providing an unenforced code style. 2017-02-27 22:04 GMT+01:00 Greg Hogan <[hidden email]>: > There was also … > > - create a flink style (for example, leaving indentation as +1 tab rather > than +2 spaces as in google's style) > > - create a flink style but optional and unenforced (but recommended for > new contributions) > > Flink currently has a reasonably consistent code style. I expect that > adopting a radically different code style module-by-module will also result > in contributions with a mix of old an new styles. If we’re not willing to > re-style flink-core today, under what circumstances will this change? With > a punctuated refactoring there would be a singular event for developers to > remember (as with the initial commits). > > Greg > > > > On Feb 27, 2017, at 3:40 PM, Dawid Wysakowicz < > [hidden email]> wrote: > > > > 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 <[hidden email] > >: > > > >> +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 <[hidden email]> 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 <[hidden email] > > > >>> 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 <[hidden email]> > >> 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 < > >>>>> [hidden email]> 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 < > >>> [hidden email] > >>>>> > >>>>>> 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 <[hidden email]> > >>>>> 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 < > >>>>>>>> [hidden email]> 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 <[hidden email]>: > >>>>>>>>> > >>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > >>>>> [hidden email] > >>>>>>> > >>>>>>>>> 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 ;-)). > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > |
Re: [DISCUSS] Code style / checkstyle
|
|
By the way, I also don't see the benefit of doing the transition piece by
piece. On Mon, 27 Feb 2017 at 22:21 Dawid Wysakowicz <[hidden email]> wrote: > I agree with adopting a custom codestyle/checkstyle for flink, but as I > understood correctly most people agree there is no point of providing an > unenforced code style. > > 2017-02-27 22:04 GMT+01:00 Greg Hogan <[hidden email]>: > > > There was also … > > > > - create a flink style (for example, leaving indentation as +1 tab rather > > than +2 spaces as in google's style) > > > > - create a flink style but optional and unenforced (but recommended for > > new contributions) > > > > Flink currently has a reasonably consistent code style. I expect that > > adopting a radically different code style module-by-module will also > result > > in contributions with a mix of old an new styles. If we’re not willing to > > re-style flink-core today, under what circumstances will this change? > With > > a punctuated refactoring there would be a singular event for developers > to > > remember (as with the initial commits). > > > > Greg > > > > > > > On Feb 27, 2017, at 3:40 PM, Dawid Wysakowicz < > > [hidden email]> wrote: > > > > > > 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 < > [hidden email] > > >: > > > > > >> +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 <[hidden email]> > 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 < > [hidden email] > > > > > >>> 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 <[hidden email]> > > >> 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 < > > >>>>> [hidden email]> 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 < > > >>> [hidden email] > > >>>>> > > >>>>>> 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 <[hidden email]> > > >>>>> 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 < > > >>>>>>>> [hidden email]> 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 <[hidden email]>: > > >>>>>>>>> > > >>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > > >>>>> [hidden email] > > >>>>>>> > > >>>>>>>>> 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 ;-)). > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > |
Re: [DISCUSS] Code style / checkstyle
|
|
In reply to this post by Stavros Kontopoulos
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 < [hidden email]> 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 <[hidden email]> 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 <[hidden email]> > > 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 <[hidden email]> > 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 < > > > > [hidden email]> 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 < > > [hidden email] > > > > > > > > > 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 <[hidden email]> > > > > 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 < > > > > > > > [hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > > > > [hidden email] > > > > > > > > > > > > > > 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 ;-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > |
Re: [DISCUSS] Code style / checkstyle
|
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 <[hidden email]> 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 < > [hidden email]> 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 <[hidden email]> 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 <[hidden email] > > > > > 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 <[hidden email]> > > 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 < > > > > > [hidden email]> 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 < > > > [hidden email] > > > > > > > > > > > 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 < > [hidden email]> > > > > > 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 < > > > > > > > > [hidden email]> 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 <[hidden email]>: > > > > > > > > > > > > > > > > > > > On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > > > > > [hidden email] > > > > > > > > > > > > > > > > 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 ;-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > |
Re: [DISCUSS] Code style / checkstyle
|
|
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. 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 <[hidden email]> 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 <[hidden email]> > 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 < >> [hidden email]> 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 <[hidden email]> 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 <[hidden email] >>> >>>> 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 <[hidden email]> >>> 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 < >>>>>> [hidden email]> 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 < >>>> [hidden email] >>>>>> >>>>>>> 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 < >> [hidden email]> >>>>>> 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 < >>>>>>>>> [hidden email]> 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 <[hidden email]>: >>>>>>>>>> >>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>> [hidden email] >>>>>>>> >>>>>>>>>> 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 ;-)). >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> |
Re: [DISCUSS] Code style / checkstyle
|
|
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 <[hidden email]> 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. 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 <[hidden email]> 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 <[hidden email]> >> 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 < >>> [hidden email]> 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 <[hidden email]> 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 <[hidden email] >>>> >>>>> 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 <[hidden email]> >>>> 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 < >>>>>>> [hidden email]> 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 < >>>>> [hidden email] >>>>>>> >>>>>>>> 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 < >>> [hidden email]> >>>>>>> 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 < >>>>>>>>>> [hidden email]> 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 <[hidden email]>: >>>>>>>>>>> >>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>> [hidden email] >>>>>>>>> >>>>>>>>>>> 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 ;-)). >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> > |
Re: [DISCUSS] Code style / checkstyle
|
|
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 <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> >>> 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 < >>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]> >>>>> >>>>>> 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 <[hidden email] <mailto:[hidden email]>> >>>>> 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 < >>>>>>>> [hidden email] <mailto:[hidden email]>> 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 < >>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>> >>>>>>>>> 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 < >>>> [hidden email] <mailto:[hidden email]>> >>>>>>>> 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 < >>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>>: >>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>> >>>>>>>>>>>> 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 ;-)). >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >> > |
Re: [DISCUSS] Code style / checkstyle
|
|
In reply to this post by Stephan Ewen
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 <[hidden email]> 写道： > > 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 <[hidden email]> > 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 < >> [hidden email]> 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 <[hidden email]> 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 <[hidden email] >>> >>>> 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 <[hidden email]> >>> 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 < >>>>>> [hidden email]> 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 < >>>> [hidden email] >>>>>> >>>>>>> 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 < >> [hidden email]> >>>>>> 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 < >>>>>>>>> [hidden email]> 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 <[hidden email]>: >>>>>>>>>> >>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>> [hidden email] >>>>>>>> >>>>>>>>>> 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 ;-)). >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> |
Re: [DISCUSS] Code style / checkstyle
|
|
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 <[hidden email]> 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 <[hidden email]> 写道： >> >> 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 <[hidden email]> >> 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 < >>> [hidden email]> 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 <[hidden email]> 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 <[hidden email] >>>> >>>>> 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 <[hidden email]> >>>> 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 < >>>>>>> [hidden email]> 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 < >>>>> [hidden email] >>>>>>> >>>>>>>> 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 < >>> [hidden email]> >>>>>>> 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 < >>>>>>>>>> [hidden email]> 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 <[hidden email]>: >>>>>>>>>>> >>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>> [hidden email] >>>>>>>>> >>>>>>>>>>> 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 ;-)). >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> > |
Re: [DISCUSS] Code style / checkstyle
|
|
In reply to this post by Aljoscha Krettek-2
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 <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> >>>> 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 < >>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]> >>>>>>> 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 <[hidden email] <mailto:[hidden email]>> >>>>>> 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 < >>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 < >>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>> 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 < >>>>> [hidden email] <mailto:[hidden email]>> >>>>>>>>> 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 < >>>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>>: >>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>>>> 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 ;-)). >>>>>>>>>>>>>> > |
Re: [DISCUSS] Code style / checkstyle
|
|
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> >>>>> 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 < >>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]> >>>>>>>> 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 <[hidden email] <mailto:[hidden email]>> >>>>>>> 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 < >>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 < >>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>> 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 < >>>>>> [hidden email] <mailto:[hidden email]>> >>>>>>>>>> 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 < >>>>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>>: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>>>>> 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 ;-)). >>>>>>>>>>>>>>> >> > |
Re: [DISCUSS] Code style / checkstyle
|
|
+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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> >>>>>> 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 < >>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]> >>>>>>>>> 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 <[hidden email] <mailto:[hidden email]>> >>>>>>>> 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 < >>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 < >>>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>>> 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 < >>>>>>> [hidden email] <mailto:[hidden email]>> >>>>>>>>>>> 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 < >>>>>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>>: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>>>>>> 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 ;-)). >>>>>>>>>>>>>>>> > |
Re: [DISCUSS] Code style / checkstyle
|
|
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> >>>>>>> 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 < >>>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]> >>>>>>>>>> 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 <[hidden email] <mailto:[hidden email]>> >>>>>>>>> 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 < >>>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 < >>>>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>>>> 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 < >>>>>>>> [hidden email] <mailto:[hidden email]>> >>>>>>>>>>>> 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 < >>>>>>>>>>>>>>> [hidden email] <mailto:[hidden email]>> 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 <[hidden email] <mailto:[hidden email]>>: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < >>>>>>>>>>>> [hidden email] <mailto:[hidden email]> >>>>>>>>>>>>>>>> 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 ;-)). >>>>>>>>>>>>>>>>> >> > |
Re: [DISCUSS] Code style / checkstyle
|
|
Cool! So, it begins =)
- Henry On Wed, Apr 26, 2017 at 7:30 AM, Aljoscha Krettek <[hidden email]> wrote: > 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> > 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 <[hidden email] > <mailto:[hidden email]>> 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 <[hidden email] <mailto: > [hidden email]>> 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 < > [hidden email] <mailto:[hidden email]>> > >>>>>>> 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 < > >>>>>>>> [hidden email] <mailto:[hidden email]>> > 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 <[hidden email] > <mailto:[hidden email]>> 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 < > [hidden email] <mailto:[hidden email]> > >>>>>>>>>> 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 > >>>>>>>> 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 < > [hidden email] <mailto:[hidden email]>> > >>>>>>>>> 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 < > >>>>>>>>>>>> [hidden email] <mailto:alexander.s. > [hidden email]>> 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 < > >>>>>>>>>> [hidden email] <mailto:[hidden email]> > >>>>>>>>>>>>> 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 < > >>>>>>>> [hidden email] <mailto:[hidden email]>> > >>>>>>>>>>>> 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 < > >>>>>>>>>>>>>>> [hidden email] <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 <[hidden email] > <mailto:[hidden email]>>: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske < > >>>>>>>>>>>> [hidden email] <mailto:[hidden email]> > >>>>>>>>>>>>>>>> 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 ;-)). > >>>>>>>>>>>>>>>>> > >> > > > > |
«
Return to (DEPRECATED) Apache Flink Mailing List archive.
|
1 view|%1 views
| Free forum by Nabble | Edit this page |