zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Enrico Olivelli <eolive...@gmail.com>
Subject Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Sat, 29 Jun 2019 06:19:16 GMT
Il sab 29 giu 2019, 07:59 Zili Chen <wander4096@gmail.com> ha scritto:

> 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.


This is quite important because we are using inlt in order to prevent
@author tags.
This was part of the ant based precommit job

Enrico




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