lol looking forward to your patch.
Best, tison. Justin Ling Mao <[email protected]> 于2019年7月19日周五 下午3:18写道： > @Zili don't worry about anything.I'am here.My first-shot patch will be > coming at this weekend, then you can help me review it. > ----- Original Message ----- > From: Zili Chen <[email protected]> > To: DevZooKeeper <[email protected]> > Subject: Re: Re: Clean up the all the checkstyle violations in the > zookeeper-server module > Date: 2019-07-19 06:04 > > Hi Enrico, > I see your concern. My question is, how does "a new execution of > checkstyle" > implement? My experience with checkstyle plugin is one pass to check all > rules so I'm curiously whether we add another profile to execute or > in other ways. > Best, > tison. > Enrico Olivelli <[email protected]> 于2019年7月18日周四 下午8:46写道： > > Hi Tison, > > > > > > > > Il gio 18 lug 2019, 10:40 Zili Chen <[email protected]> ha scritto: > > > > > Hi Enrico, > > > > > > I just noticed that you have mentioned > > > > > > 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'm curious how "a separated execution" achieves? I found > > > that checkstyle plugin can be only configured one checkstyle > > > conifg files and thus propose the current simple config applied > > > on all modules and a strict config covering the simple one > > > applied per package(server, prometheus, and jute as discussed). > > > > > > > > > We already have one checkstyle file and it must be checked against the > > whole codebase, ad it looks for @author tags. > > > > If we add new rules to the existing file they will be enabled for all of > > the modules. > > If you limit checkstyle to only some package we will lose current checks > > > > So my idea is to add a new execution of checkstyle with a new checkstyle > > configuration file, for this new execution of the plugin we will have: > > - strict checks > > - selective per-package abilitation of checkstyle > > > > > > > > > > > > Best, > > > tison. > > > > > > > > > Enrico Olivelli <[email protected]> 于2019年7月6日周六 下午8:20写道： > > > > > > > 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 <[email protected]> > > 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 <[email protected]> > > > > > To: DevZooKeeper <[email protected]> > > > > > 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 <[email protected]> > > 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 <[email protected]> 于2019年7月1日周一 下午9:09写道： > > > > > > > > > > > > > I like the idea of doing this in iterations. > > > > > > > > > > > > > > Andor > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 2019. Jun 29., at 8:35, Zili Chen <[email protected]> > > > 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 <[email protected]> 于2019年6月29日周六 > 下午2:19写道： > > > > > > > > > > > > > > > >> Il sab 29 giu 2019, 07:59 Zili Chen <[email protected]> > 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 <[email protected]> 于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 > > > > > > <[email protected] > > > > > > > >>> > > > > > > > >>> 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 < > > > > > > [email protected] > > > > > > > >>> > > > > > > > >>>>> 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 < > > > > > > > >> [email protected] > > > > > > > >>>> > > > > > > > >>>> 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. > > > > > > > >>>>>> > > > > > > > >>>>> > > > > > > > >>>> > > > > > > > >>> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
