zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zili Chen <wander4...@gmail.com>
Subject Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Sat, 29 Jun 2019 05:58:36 GMT
Thank you Enrico. It seems my previous reply delivered
into another thread. Repost below.

Hi zookeepers,

I have proceeded ZOOKEEPER-3446 and been guided to here
to discuss for a consensus before any more efforts.

In general, +1 on introducing and forcing checkstyle.

About the process, I agree that we firstly reach a consensus
on the configuration and enable it per package. In order for
our contributors to rebase as few times as possible, we'd
better introduce all rules we all agree on at once. Note that
we could always add rule if someone ask for and agreed by the
community.

Currently, we turn on checkstyle on all modules. Following the
process above, we firstly turn off it once we apply the new
configuration, and then turn on it per package.

If the community is willing to do this work, a JIRA about the new
checkstyle configuration should be filed and we continue the discussion
there. Generally, rules proposed by Enrico are good start point and
I agree on we should not introduce anything "fancy", but according to
what is actually needed.

Best,
tison.


Enrico Olivelli <eolivelli@gmail.com> 于2019年6月29日周六 下午1:51写道:

> @Zili Chen
> This is the original email thread. So you can answer here.
>
> Enrico
>
> Il ven 28 giu 2019, 11:04 Norbert Kalmar <nkalmar@cloudera.com.invalid> ha
> scritto:
>
> > Community is eager to jump on on this, we already have pull requests
> > cleaning up imports :)
> >
> > First of all, sorry for the late reply (I thought I already answered
> this,
> > I remember reading it and drawing up an answer. Oh well)
> >
> > Some big patches are already reviewed, I think we should commit as much
> as
> > possible before doing this refactor. (I'll also try to rev up my code
> > review/commit thread)
> >
> > As for waiting for 3.6.0 - I don't see the reason we should. Unless of
> > course this would delay the release too much...
> >
> > I haven't checked HBase checkstyle against our code, I don't think we
> > should introduce anything "fancy". What Enrico listed up sounds like a
> good
> > starting point.
> >
> > +1 on introducing and forcing checkstyle.
> >
> > Regards,
> > Norbert
> >
> >
> > On Sun, Jun 23, 2019 at 7:27 PM Enrico Olivelli <eolivelli@gmail.com>
> > wrote:
> >
> > > Justin,
> > > Thank you so much for your help in this.
> > >
> > > I would suggest to apply all of the rules in one pass, splitting the
> work
> > > per package.
> > > This way reviews will be easier, we will limit the number of commits
> and
> > we
> > > won't annoy too much the contributors , asking for hard rebases
> > >
> > > This is how we did it on Apache Bookkeeper
> > > https://github.com/apache/bookkeeper/issues/230
> > >
> > > I will help review and commit all of your patches,  it will be mostly a
> > > matter of code reformat without any behavior change.
> > > Currently I am doing the same kind of work on others projects of my
> > > company, so I perfectly know how the work will go.
> > >
> > > Before starting we must ensure that:
> > > 1) community is willing to do this work (we will force a rebase on
> mostly
> > > every pending PR)
> > > 2) the proposed configuration is accepted by the community
> > > 3) it is the good time to do it, or should we wait for 3.6.0 to be out
> > >
> > >
> > > I see you are referring to hbase checkstyle file,  did  you already
> > checked
> > > how much different it is from current project style?
> > > Will we only need to remove trailing spaces, reorder members, fix
> > imports,
> > > cut long lines ?
> > >
> > >
> > > Cheers
> > > Enrico
> > >
> > >
> > >
> > > Il dom 23 giu 2019, 15:11 Justin Ling Mao <maoling199210191@sina.com>
> ha
> > > scritto:
> > >
> > > > Background:zookeeper-server is the main-module of the zk codebase.
> > > > Unfortunately, there were many checkstyle violations in it. To
> improve
> > > the
> > > > code quality and code standards,
> > > > IMHO, it's time to clean up the all the checkstyle violations(turn on
> > the
> > > > <failOnViolation>true</failOnViolation>). we can learn from
the hbase
> > > whose
> > > > checkstyle(almost 40+ rules) is very strict and ensures a very
> unified
> > > code
> > > > style.(
> > > >
> > >
> >
> https://github.com/apache/hbase/blob/master/hbase-checkstyle/src/main/resources/hbase/checkstyle.xml
> > > > )
> > > > My planing is: clean up the all the checkstyle one rule by another to
> > > > avoid too much code changes for review.
> > > > Everything's hard in the beginning, I have fired my first shot(
> > > > https://github.com/apache/zookeeper/pull/992).
> > > > If this draft has accepted by the community, I will create the
> > > > corresponding sub-tasks for more people joining this work.
> > > >
> > > > Cited the comment from Enrico Olivelli in the
> > > >
> > >
> >
> ZOOKEEPER-3431:--------------------------------------------------------------we
> > > > have to discuss this topic with the community.I have experience on
> > > > BookKeeper, we had to clean up groups of packages.This is the kind of
> > > stuff
> > > > that invalidates all of the pending pull requests.I have already sett
> > up
> > > a
> > > > basic checkstyle configuration file and it is already active but it
> is
> > > > performing only very basic checks (like no 'author' tags).I will
> > > appreciate
> > > > very much if you want to drive this effort, personally I would start
> > this
> > > > stuff after 3.6.0 release, once we consolidate current master and the
> > > maven
> > > > build. I would have sent an email on the dev@ list soon.We also have
> > to
> > > > agree on the checkstyle configuration, it is not trivial, I would
> take
> > > the
> > > > file from HBase, BookKeeper or other ASF projects on the Haddoop
> > > ecosystem.
> > >
> >
>

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