zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andor Molnar <an...@apache.org>
Subject Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Mon, 01 Jul 2019 13:09:41 GMT
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
View raw message