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