From dev-return-81810-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Mon Jul 1 13:09:45 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id D2901180670 for ; Mon, 1 Jul 2019 15:09:44 +0200 (CEST) Received: (qmail 18924 invoked by uid 500); 1 Jul 2019 13:09:43 -0000 Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list dev@zookeeper.apache.org Received: (qmail 18913 invoked by uid 99); 1 Jul 2019 13:09:43 -0000 Received: from Unknown (HELO mailrelay1-lw-us.apache.org) (10.10.3.159) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 01 Jul 2019 13:09:43 +0000 Received: from [172.30.65.194] (unknown [185.63.45.212]) by mailrelay1-lw-us.apache.org (ASF Mail Server at mailrelay1-lw-us.apache.org) with ESMTPSA id E9E1B8174 for ; Mon, 1 Jul 2019 13:09:42 +0000 (UTC) From: Andor Molnar Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: Clean up the all the checkstyle violations in the zookeeper-server module Date: Mon, 1 Jul 2019 15:09:41 +0200 References: <20190623131048.33E1F19000E1@webmail.sinamail.sina.com.cn> To: DevZooKeeper In-Reply-To: Message-Id: X-Mailer: Apple Mail (2.3445.104.11) I like the idea of doing this in iterations. Andor > On 2019. Jun 29., at 8:35, Zili Chen wrote: >=20 > 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. >=20 > Best, > tison. >=20 >=20 > Enrico Olivelli =E4=BA=8E2019=E5=B9=B46=E6=9C=8829= =E6=97=A5=E5=91=A8=E5=85=AD =E4=B8=8B=E5=8D=882:19=E5=86=99=E9=81=93=EF=BC= =9A >=20 >> Il sab 29 giu 2019, 07:59 Zili Chen ha = scritto: >>=20 >>> Thank you Enrico. It seems my previous reply delivered >>> into another thread. Repost below. >>>=20 >>> Hi zookeepers, >>>=20 >>> I have proceeded ZOOKEEPER-3446 and been guided to here >>> to discuss for a consensus before any more efforts. >>>=20 >>> In general=EF=BC=8C +1 on introducing and forcing checkstyle. >>>=20 >>> 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. >>>=20 >>> Currently, we turn on checkstyle on all modules. >>=20 >>=20 >> This is quite important because we are using inlt in order to prevent >> @author tags. >> This was part of the ant based precommit job >>=20 >> Enrico >>=20 >>=20 >>=20 >>=20 >> Following the >>> process above, we firstly turn off it once we apply the new >>> configuration, and then turn on it per package. >>>=20 >>> 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. >>>=20 >>> Best, >>> tison. >>>=20 >>>=20 >>> Enrico Olivelli =E4=BA=8E2019=E5=B9=B46=E6=9C=88= 29=E6=97=A5=E5=91=A8=E5=85=AD =E4=B8=8B=E5=8D=881:51=E5=86=99=E9=81=93=EF=BC= =9A >>>=20 >>>> @Zili Chen >>>> This is the original email thread. So you can answer here. >>>>=20 >>>> Enrico >>>>=20 >>>> Il ven 28 giu 2019, 11:04 Norbert Kalmar = >>=20 >>> ha >>>> scritto: >>>>=20 >>>>> Community is eager to jump on on this, we already have pull = requests >>>>> cleaning up imports :) >>>>>=20 >>>>> 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) >>>>>=20 >>>>> 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) >>>>>=20 >>>>> 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... >>>>>=20 >>>>> 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. >>>>>=20 >>>>> +1 on introducing and forcing checkstyle. >>>>>=20 >>>>> Regards, >>>>> Norbert >>>>>=20 >>>>>=20 >>>>> On Sun, Jun 23, 2019 at 7:27 PM Enrico Olivelli = >>=20 >>>>> wrote: >>>>>=20 >>>>>> Justin, >>>>>> Thank you so much for your help in this. >>>>>>=20 >>>>>> 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 >>>>>>=20 >>>>>> This is how we did it on Apache Bookkeeper >>>>>> https://github.com/apache/bookkeeper/issues/230 >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> 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 >>>>>>=20 >>>>>>=20 >>>>>> 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 ? >>>>>>=20 >>>>>>=20 >>>>>> Cheers >>>>>> Enrico >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> Il dom 23 giu 2019, 15:11 Justin Ling Mao < >> maoling199210191@sina.com >>>>=20 >>>> ha >>>>>> scritto: >>>>>>=20 >>>>>>> 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 >>>>>>> true). we can learn from the >>> hbase >>>>>> whose >>>>>>> checkstyle(almost 40+ rules) is very strict and ensures a very >>>> unified >>>>>> code >>>>>>> style.( >>>>>>>=20 >>>>>>=20 >>>>>=20 >>>>=20 >>>=20 >> = https://github.com/apache/hbase/blob/master/hbase-checkstyle/src/main/reso= urces/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. >>>>>>>=20 >>>>>>> Cited the comment from Enrico Olivelli in the >>>>>>>=20 >>>>>>=20 >>>>>=20 >>>>=20 >>>=20 >> = 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. >>>>>>=20 >>>>>=20 >>>>=20 >>>=20 >>=20