zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Enrico Olivelli <eolive...@gmail.com>
Subject Re: Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Sat, 06 Jul 2019 12:19:53 GMT
Justin,
This is how we did it in Bookkeeper, we enabled checkstyle only for group
of packages in the main module (the biggest one, bookkeeper-server)

https://github.com/apache/bookkeeper/issues/230

I suggest using that checkstyle config, I feel we won't have so many
violations.

We can keep current checkstyle invokation that checks for @author tags as a
separate 'execution' of the plugin with a specific checkstyle file (as you
already said)

I am happy to help, thank you for driving this effort

Enrico


Il sab 6 lug 2019, 11:33 Justin Ling Mao <maoling199210191@sina.com> ha
scritto:

> - 1.It seems that we had reached a consensus to work on this.- 2.I also
> agree on the way: fix one package at a time, then another.- 3.Now Let us
> discuss some details:    - 3.1 how to make the checkstyle only check the
> package we specify? I found this:      URL:
> https://stackoverflow.com/questions/26455174/only-enable-some-checks-for-certain-inner-package
>     @Olivelli Could you give me more your insight?    - 3.2 What rules will
> we init in the checkstyle.xml?       3.2.1 - I also think the rules from
> the hbase is too strict which will cause too many,many violations.
>  3.2.2 - apply the "Google's Java Style Checkstyle Coverage" is a good
> option? which seems to be more simplify and more suitable for us?
>      URL:https://checkstyle.sourceforge.io/google_style.html
>
>
>
> ----- Original Message -----
> From: Andor Molnar <andor@cloudera.com.INVALID>
> To: DevZooKeeper <dev@zookeeper.apache.org>
> Subject: Re: Clean up the all the checkstyle violations in the
> zookeeper-server module
> Date: 2019-07-02 13:22
>
> Yes. That way we only need to fix one package at a time.
> Andor
> On Mon, Jul 1, 2019 at 4:10 PM Zili Chen <wander4096@gmail.com> wrote:
> > Hi Andor,
> >
> > To be exact, "iterations" means we define the original rules
> > in checkstyle configuration at once and turn them on one package
> > after another, so iterations. Is it correct?
> >
> > Best,
> > tison.
> >
> >
> > Andor Molnar <andor@apache.org> 于2019年7月1日周一 下午9:09写道:
> >
> > > I like the idea of doing this in iterations.
> > >
> > > Andor
> > >
> > >
> > >
> > > > On 2019. Jun 29., at 8:35, Zili Chen <wander4096@gmail.com> wrote:
> > > >
> > > > A solution could be, we remains current simple configuration
> > > > and introduce a so-called "strict-checkstyle.xml" and apply
> > > > it per package. Once we enable it on every package, we can
> > > > merge it into the simple configuration.
> > > >
> > > > Best,
> > > > tison.
> > > >
> > > >
> > > > Enrico Olivelli <eolivelli@gmail.com> 于2019年6月29日周六
下午2:19写道:
> > > >
> > > >> 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