flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Henry Saputra <henry.sapu...@gmail.com>
Subject Re: [DISCUSS] Code style / checkstyle
Date Thu, 27 Apr 2017 04:34:24 GMT
Cool! So, it begins =)

- Henry

On Wed, Apr 26, 2017 at 7:30 AM, Aljoscha Krettek <aljoscha@apache.org>
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 <chesnay@apache.org> wrote:
> >
> > +1 to use earth rotation as the new standard time unit. Maybe more
> importantly, I'm absolutely in favor of merging it.
> >
> > On 18.04.2017 20:39, Aljoscha Krettek wrote:
> >> I rebased the PR [1] on current master. Is there any strong objection
> against merging this (minus the two last commits which introduce
> curly-brace-style checking). If not, I would like to merge this after two
> earth rotations, i.e. after all the time zones have had some time to react.
> >>
> >> The complete set of checks has been listed by Chesnay (via Greg) before
> but the gist of it is that we only add common-sense checks that most people
> should be able to agree upon so that we avoid edit wars (especially when it
> comes to whitespace, import order and Javadoc paragraph styling).
> >>
> >> [1] https://github.com/apache/flink/pull/3567
> >>> On 5. Apr 2017, at 23:54, Chesnay Schepler <chesnay@apache.org> wrote:
> >>>
> >>> I agree to just allow both. While I definitely prefer 1) i can see why
> someone might prefer 2).
> >>>
> >>> Wouldn't want to delay this anymore; can't find to add this to
> flink-metrics and flink-python...
> >>>
> >>> On 03.04.2017 18:33, Aljoscha Krettek wrote:
> >>>> I think enough people did already look at the checkstyle rules
> proposed in the PR.
> >>>>
> >>>> On most of the rules reaching consensus is easy so I propose to
> enable all rules except those regarding placement of curly braces and
> control flow formatting. The two styles in the Flink code base are:
> >>>>
> >>>> 1)
> >>>> if () {
> >>>> } else {
> >>>> }
> >>>>
> >>>> try {
> >>>> } catch () {
> >>>> }
> >>>>
> >>>> and
> >>>>
> >>>> 2)
> >>>>
> >>>> if () {
> >>>> }
> >>>> else {
> >>>> }
> >>>>
> >>>> try {
> >>>> }
> >>>> catch () {
> >>>> }
> >>>>
> >>>> I think it’s hard to reach consensus on these so I suggest to keep
> allowing both styles.
> >>>>
> >>>> Any comments very welcome! :-)
> >>>>
> >>>> Best,
> >>>> Aljoscha
> >>>>> On 19. Mar 2017, at 17:09, Aljoscha Krettek <aljoscha@apache.org>
> wrote:
> >>>>>
> >>>>> I played around with this over the week end and it turns out that
> the required changes in flink-streaming-java are not that big. I created a
> PR with a proposed checkstyle.xml and the required code changes:
> https://github.com/apache/flink/pull/3567 <https://github.com/apache/
> flink/pull/3567>. There’s a longer description of the style in the PR.
> The commits add continuously more invasive changes so we can start with the
> more lightweight changes if we want to.
> >>>>>
> >>>>> If we want to go forward with this I would also encourage other
> people to use this for different modules and see how it turns out.
> >>>>>
> >>>>> Best,
> >>>>> Aljoscha
> >>>>>
> >>>>>> On 18 Mar 2017, at 08:00, Aljoscha Krettek <aljoscha@apache.org
> <mailto:aljoscha@apache.org>> wrote:
> >>>>>>
> >>>>>> I added an issue for adding a custom checkstyle.xml for
> flink-streaming-java so that we can gradually add checks:
> https://issues.apache.org/jira/browse/FLINK-6107 <
> https://issues.apache.org/jira/browse/FLINK-6107>. I outlined the
> procedure in the Jira. We can use this as a pilot project and see how it
> goes and then gradually also apply to other modules.
> >>>>>>
> >>>>>> What do you think?
> >>>>>>
> >>>>>>> On 6 Mar 2017, at 12:42, Stephan Ewen <sewen@apache.org
<mailto:
> sewen@apache.org>> wrote:
> >>>>>>>
> >>>>>>> A singular "all reformat in one instant" will cause immense
damage
> to the
> >>>>>>> project, in my opinion.
> >>>>>>>
> >>>>>>> - There are so many pull requests that we are having a hard
time
> keeping
> >>>>>>> up, and merging will a lot more time intensive.
> >>>>>>> - I personally have many forked branches with WIP features
that
> will
> >>>>>>> probably never go in if the branches become unmergeable.
I expect
> that to
> >>>>>>> be true for many other committers and contributors.
> >>>>>>> - Some companies have Flink forks and are rebasing patches
onto
> master
> >>>>>>> regularly. They will be completely screwed by a full reformat.
> >>>>>>>
> >>>>>>> If we do something, the only thing that really is possible
is:
> >>>>>>>
> >>>>>>> (1) Define a style. Ideally not too far away from Flink's
style.
> >>>>>>> (2) Apply it to new projects/modules
> >>>>>>> (3) Coordinate carefully to pull it into existing modules,
module
> by
> >>>>>>> module. Leaving time to adopt pull requests bit by bit,
and
> allowing forks
> >>>>>>> to go through minor merges, rather than total conflict.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On Wed, Mar 1, 2017 at 5:57 PM, Henry Saputra <
> henry.saputra@gmail.com <mailto:henry.saputra@gmail.com>>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> It is actually Databricks Scala guide to help contributions
to
> Apache Spark
> >>>>>>>> so not really official Spark Scala guide.
> >>>>>>>> The style guide feels bit more like asking people to
write Scala
> in Java
> >>>>>>>> mode so I am -1 to follow the style for Apache Flink
Scala if
> that what you
> >>>>>>>> are recommending.
> >>>>>>>>
> >>>>>>>> If the "unification" means ONE style guide for both
Java and
> Scala I would
> >>>>>>>> vote -1 to it because both languages have different
semantics and
> styles to
> >>>>>>>> make them readable and understandable.
> >>>>>>>>
> >>>>>>>> We could start with improving the Scala maven style
guide to
> follow more
> >>>>>>>> Scala official style guide [1] and add IntelliJ Idea
or Eclipse
> plugin
> >>>>>>>> style to follow suit.
> >>>>>>>>
> >>>>>>>> Java side has bit more strict style check but we could
make it
> tighter but
> >>>>>>>> embracing more Google Java guide closely with minor
exceptions
> >>>>>>>>
> >>>>>>>> - Henry
> >>>>>>>>
> >>>>>>>> [1] http://docs.scala-lang.org/style/ <
> http://docs.scala-lang.org/style/>
> >>>>>>>>
> >>>>>>>> On Mon, Feb 27, 2017 at 11:54 AM, Stavros Kontopoulos
<
> >>>>>>>> st.kontopoulos@gmail.com <mailto:st.kontopoulos@gmail.com>>
> wrote:
> >>>>>>>>
> >>>>>>>>> +1 to provide and enforcing a unified code style
for both java
> and scala.
> >>>>>>>>> Unification should apply when it makes sense like
comments
> though.
> >>>>>>>>>
> >>>>>>>>> Eventually code base should be re-factored. I would
vote for the
> one at a
> >>>>>>>>> time module fix apporoach.
> >>>>>>>>> Style guide should be part of any PR review.
> >>>>>>>>>
> >>>>>>>>> We could also have a look at the spark style guide:
> >>>>>>>>> https://github.com/databricks/scala-style-guide
<
> https://github.com/databricks/scala-style-guide>
> >>>>>>>>>
> >>>>>>>>> The style code and general guidelines help keep
code more
> readable and
> >>>>>>>> keep
> >>>>>>>>> things simple
> >>>>>>>>> with many contributors and different styles of code
writing +
> language
> >>>>>>>>> features.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Mon, Feb 27, 2017 at 8:01 PM, Stephan Ewen <sewen@apache.org
> <mailto: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 <mailto: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
<
> 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 <mailto: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 <mailto: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 <mailto: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 <mailto: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
<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 <uce@apache.org
> <mailto:uce@apache.org>>:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On Fri, Feb 24,
2017 at 10:46 AM, Fabian Hueske <
> >>>>>>>>>>>> fhueske@gmail.com <mailto: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