flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fabian Hueske <fhue...@gmail.com>
Subject Re: [DISCUSS] Code style / checkstyle
Date Fri, 24 Feb 2017 09:46:54 GMT
We have discussed this issue several times in the past and never got around
to do it.

Although I agree that it would be nice to have a stricter code style, I
don't think we should introduce a code style.
IMO, at this point the costs (losing the history, PR hassle, etc.) would be
too high compared to the benefits (that I am aware of).

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.

On the other hand, it's not clear to me how much we would gain with a code
style and what's the cost of not having one.

Best, Fabian





2017-02-23 17:52 GMT+01:00 Jinkui Shi <shijinkui666@163.com>:

> Thanks to discuss this problem again.
> 1. Google checkstyle is good for java.
> 2. scala check style is here [1]
> 3. We can make a Flink plan contain issues, one sub-issue one rule.
> Resolve this in short time.
>
> Current code style may be historical accumulate. If we don’t normalize the
> code step by step, new code will also follow bad style without strict
> checking.
> We can adjust the exist code once one rule. But there will be lots of
> conflict in the not merged pull requests. Need all the pull request resolve
> the conflict manually.
> Resolve long term trouble in short time:)
>
>
> [1]  http://www.scalastyle.org/ <http://www.scalastyle.org/>
> > On Feb 23, 2017, at 23:31, Aljoscha Krettek <aljoscha@apache.org> wrote:
> >
> > If we go for a codestyle/checkstyle I would suggest to use the Google
> > style. This already has checkstyle, IntelliJ style, Eclipse style and a
> > code formatting tool and is well established. However, some people will
> not
> > like this style. In general, I think we will never manage to find a style
> > that all people will like.
> >
> > On Wed, 22 Feb 2017 at 18:36 Dawid Wysakowicz <
> wysakowicz.dawid@gmail.com>
> > wrote:
> >
> >> So how about preparing a code style and corresponding checkstyle and
> >> enabling it gradually module by module whenever shepherd/commiter with
> >> expertise in a module will have time to introduce/check such a change?
> >> Maybe it will make the "snowball effect" happen?
> >> I agree there is no point in preparing code style/checkstyle until it
> will
> >> be introduced somewhere. I will be willing to work on the checkstyle if
> >> some volunteering modules appear.
> >>
> >> 2017-02-22 17:09 GMT+01:00 Chesnay Schepler <chesnay@apache.org>:
> >>
> >>> For file where we don't enforce checkstyle things should work they way
> >>> they do right now.
> >>>
> >>> Turn off auto-formatting, and only format code that you touched and
> >> that's
> >>> it. For these
> >>> modification we will have to check them manually in the PRs as we do
> now.
> >>>
> >>>
> >>> On 22.02.2017 16:22, Greg Hogan wrote:
> >>>
> >>>> Will not the code style be applied on save to any user-modified file?
> So
> >>>> this will clutter PRs and overwrite history.
> >>>>
> >>>> On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz <
> >>>> wysakowicz.dawid@gmail.com> wrote:
> >>>>
> >>>> I also agree with Till and Chesnayl. Anyway as to "capture the current
> >>>>> style" I have some doubts if this is possible, as it changes file
to
> >>>>> file.
> >>>>>
> >>>>> Chesnay's suggestion as to were enforce the checkstyle seems
> reasonable
> >>>>> to
> >>>>> me, but I am quite new to the community :).
> >>>>> Enabling checkstyle for particular packages is possible.
> >>>>>
> >>>>> 2017-02-22 12:07 GMT+01:00 Chesnay Schepler <chesnay@apache.org>:
> >>>>>
> >>>>> I agree with Till.
> >>>>>>
> >>>>>> I would propose enforcing checkstyle on a subset of the modules,
> >>>>>>
> >>>>> basically
> >>>>>
> >>>>>> those that are not
> >>>>>> flink-runtime, flink-java, flink-streaming-java. These are the
ones
> >> imo
> >>>>>> where messing with the history
> >>>>>> can be detrimental; for the others it isn't really important
imo.
> >>>>>> (Note that i excluded scala code since i don't know the state
of
> >>>>>> checkstyle compliance there)
> >>>>>>
> >>>>>> For flink-runtime we could maybe (don't know if it is supported)
> >> enforce
> >>>>>> checkstyle for all classes
> >>>>>> under o.a.f.migration, so that at least the flip-6 code is covered.
> >>>>>>
> >>>>>> Similarly, enforcing checkstyle for all tests should be fine
as
> well.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Chesnay
> >>>>>>
> >>>>>>
> >>>>>> On 22.02.2017 11:48, Till Rohrmann wrote:
> >>>>>>
> >>>>>> I think that not enforcing a code style is as good as not having
any
> >>>>>>>
> >>>>>> code
> >>>>>
> >>>>>> style to be honest. Having an IntelliJ or Eclipse profile is
nice
> and
> >>>>>>>
> >>>>>> some
> >>>>>
> >>>>>> people will probably use it, but my gut feeling is that the
majority
> >>>>>>>
> >>>>>> won't
> >>>>>
> >>>>>> notice it.
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>> Till
> >>>>>>>
> >>>>>>> On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi <uce@apache.org>
> >> wrote:
> >>>>>>>
> >>>>>>> Kurt's proposal sounds reasonable.
> >>>>>>>
> >>>>>>>> What about the following:
> >>>>>>>> - We try to capture the current style in an code style
> configuration
> >>>>>>>> (for IntelliJ and maybe Eclipse)
> >>>>>>>> - We provide that on the website for contributors to
download
> >>>>>>>> - We don't enforce it, but new contributions and changes
are free
> to
> >>>>>>>> format with this style as changes happen
> >>>>>>>>
> >>>>>>>> Practically speaking, this should not change much except
maybe the
> >>>>>>>> import order or whitespace after certain keywords, etc.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Wed, Feb 22, 2017 at 4:48 AM, Kurt Young <ykt836@gmail.com>
> >> wrote:
> >>>>>>>>
> >>>>>>>> +1 to provide a unified code style for both java and
scala.
> >>>>>>>>>
> >>>>>>>>> -1 to adjust all the existing code to the new style
in one step.
> >> The
> >>>>>>>>>
> >>>>>>>>> code's
> >>>>>>>>
> >>>>>>>> history contains very helpful information which can
help
> >>>>>>>>> develops to understand why these codes are added,
which jira is
> >>>>>>>>>
> >>>>>>>> related.
> >>>>>
> >>>>>> This information is too valuable to lose. I think we can
> >>>>>>>>> do the reformat thing step by step, each time when
the codes
> being
> >>>>>>>>>
> >>>>>>>>> changed,
> >>>>>>>>
> >>>>>>>> we can adopt them to the new style.
> >>>>>>>>> IMHO this is also the reason why the unified code
style is
> >> important.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Best,
> >>>>>>>>> Kurt
> >>>>>>>>>
> >>>>>>>>> On Wed, Feb 22, 2017 at 5:50 AM, Dawid Wysakowicz
<
> >>>>>>>>> wysakowicz.dawid@gmail.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>>> I would like to resurrect the discussing ([1]
> >>>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>>>>>>> nabble.com/Code-style-guideline-for-Scala-td7526.html>
> >>>>>>>>>> , [2]
> >>>>>>>>>> <http://apache-flink-mailing-list-archive.1008284.n3.
> >>>>>>>>>> nabble.com/Intellij-code-style-td11092.html>)
> >>>>>>>>>> about creating unified code style(that could
be imported to at
> >> least
> >>>>>>>>>> IntelliJ and Eclipse) and corresponding stricter
checkstyle
> rules.
> >>>>>>>>>>
> >>>>>>>>>> I know that the hardest part is to adjust the
existing code to
> the
> >>>>>>>>>>
> >>>>>>>>> new
> >>>>>
> >>>>>> checkstyle rules. Do you believe it would be worth the effort?
All
> >>>>>>>>>> suggestions are welcome.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>
> >>
>
>

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