flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dawid Wysakowicz <wysakowicz.da...@gmail.com>
Subject Re: [DISCUSS] Code style / checkstyle
Date Mon, 27 Feb 2017 21:21:32 GMT
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 <code@greghogan.com>:

> 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 <
> wysakowicz.dawid@gmail.com> 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 <st.kontopoulos@gmail.com
> >:
> >
> >> +1 to provide and enforcing a unified code style for both java and
> scala.
> >> Unification should apply when it makes sense like comments though.
> >>
> >> Eventually code base should be re-factored. I would vote for the one at
> a
> >> time module fix apporoach.
> >> Style guide should be part of any PR review.
> >>
> >> We could also have a look at the spark style guide:
> >> https://github.com/databricks/scala-style-guide
> >>
> >> The style code and general guidelines help keep code more readable and
> keep
> >> things simple
> >> with many contributors and different styles of code writing + language
> >> features.
> >>
> >>
> >> On Mon, Feb 27, 2017 at 8:01 PM, Stephan Ewen <sewen@apache.org> wrote:
> >>
> >>> I agree, reformatting 90% of the code base is tough.
> >>>
> >>> There are two main issues:
> >>>  (1) Incompatible merges. This is hard, especially for the folks that
> >> have
> >>> to merge the pull requests ;-)
> >>>
> >>>  (2) Author history: This is less of an issue, I think. "git log
> >>> <filename>" and "git show <revision> -- <filename>" will
still work and
> >> one
> >>> may have to go one commit back to find out why something was changed
> >>>
> >>>
> >>> What I could image is to do this incrementally. Define the code style
> in
> >>> "flink-parent" but do not activate it.
> >>> Then start with some projects (new projects, plus some others):
> >>> merge/reject PRs, reformat, activate code style.
> >>>
> >>> Piece by piece. This is realistically going to take a long time until
> it
> >> is
> >>> pulled through all components, but that's okay, I guess.
> >>>
> >>> Stephan
> >>>
> >>>
> >>> On Mon, Feb 27, 2017 at 1:53 PM, Aljoscha Krettek <aljoscha@apache.org
> >
> >>> 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 <trohrmann@apache.org>
> >> wrote:
> >>>>
> >>>>> No, I think that's exactly what people mean when saying "losing
the
> >>>> commit
> >>>>> history". With the reformatting you would have to go manually through
> >>> all
> >>>>> past commits until you find the commit which changed a given line
> >>> before
> >>>>> the reformatting.
> >>>>>
> >>>>> Cheers,
> >>>>> Till
> >>>>>
> >>>>> On Sun, Feb 26, 2017 at 6:32 PM, Alexander Alexandrov <
> >>>>> alexander.s.alexandrov@gmail.com> wrote:
> >>>>>
> >>>>>> Just to clarify - by "losing the commit history" you actually
mean
> >>>>> "losing
> >>>>>> the ability to annotate each line in a file with its last commit",
> >>>> right?
> >>>>>>
> >>>>>> Or is there some other sense in which something is lost after
> >>> applying
> >>>>> bulk
> >>>>>> re-format?
> >>>>>>
> >>>>>> Cheers,
> >>>>>> A.
> >>>>>>
> >>>>>> On Sat, Feb 25, 2017 at 7:10 AM Henry Saputra <
> >>> henry.saputra@gmail.com
> >>>>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Just want to clarify what unify code style here.
> >>>>>>>
> >>>>>>> Is the intention to have IDE and Maven plugins to have the
same
> >>> check
> >>>>>> style
> >>>>>>> rules?
> >>>>>>>
> >>>>>>> Or are we talking about having ONE code style for both Java
and
> >>>> Scala?
> >>>>>>>
> >>>>>>> - Henry
> >>>>>>>
> >>>>>>> On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan <code@greghogan.com>
> >>>>> wrote:
> >>>>>>>
> >>>>>>>> I agree wholeheartedly with Ufuk. We cannot reformat
the
> >>> codebase,
> >>>>>> cannot
> >>>>>>>> pause while flushing the PR queue, and won't find a
consensus
> >>> code
> >>>>>> style.
> >>>>>>>>
> >>>>>>>> I think we can create a baseline code style for new
and
> >> existing
> >>>>>>>> contributors for which reformatting on changed files
will be
> >>>>> acceptable
> >>>>>>> for
> >>>>>>>> PR reviews.
> >>>>>>>>
> >>>>>>>> On Fri, Feb 24, 2017 at 5:01 AM, Dawid Wysakowicz <
> >>>>>>>> wysakowicz.dawid@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>>> The problem with code style when it is not enforced
is that
> >> it
> >>>> will
> >>>>>> be
> >>>>>>> a
> >>>>>>>>> matter of luck to what parts of files / new files
will it be
> >>>>> applied.
> >>>>>>>> When
> >>>>>>>>> the code style is not applied to whole file, it
is pretty
> >> much
> >>>>>> useless
> >>>>>>>>> anyway. You would need to manually select just the
fragments
> >>> one
> >>>> is
> >>>>>>>>> changing. The benefits of having code style and
enforcing it
> >> I
> >>>> see
> >>>>>> are:
> >>>>>>>>> - being able to apply autoformatter, which speeds
up writing
> >>>> code
> >>>>>>>>> - it would make reviewing PRs easier as e.g. there
would be
> >>> line
> >>>>>>> length
> >>>>>>>>> limit applied etc. which will make line breaking
more reader
> >>>>>> friendly.
> >>>>>>>>>
> >>>>>>>>> Though I think if a consensus is not reachable it
would be
> >> good
> >>>> to
> >>>>>> once
> >>>>>>>> and
> >>>>>>>>> forever decide that we don't want a code style and
> >> checkstyle.
> >>>>>>>>>
> >>>>>>>>> 2017-02-24 10:51 GMT+01:00 Ufuk Celebi <uce@apache.org>:
> >>>>>>>>>
> >>>>>>>>>> On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske
<
> >>>>> fhueske@gmail.com
> >>>>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>>> I agree with Till that encouraging a code
style without
> >>>>> enforcing
> >>>>>>> it
> >>>>>>>>> does
> >>>>>>>>>>> not make a lot of sense.
> >>>>>>>>>>> If we enforce it, we need to touch all files
and PRs.
> >>>>>>>>>>
> >>>>>>>>>> I think it makes sense for new contributors
to have a
> >>> starting
> >>>>>> point
> >>>>>>>>>> without enforcing anything (I do agree that
we are past the
> >>>> point
> >>>>>> to
> >>>>>>>>>> reach consensus on a style and enforcement ;-)).
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message