Il dom 18 ago 2019, 03:26 Zili Chen <wander4...@gmail.com> ha scritto:
> Of course I volunteer to do. > Great > > To clarify the process, we want to merge checkstyle-strict.xml into > checkstyle.xml and thus turn on rules in current checkstyle-strict.xml > on project level, while also removing configurations in plugins field > of pom.xml in submodules. Is this process expected? > Yes > We don't need to apply checkstyle-strict.xml to 'contrib' it is another big work that is it not worth to do, as such Java modules are not maintained. It would be awesome to have only one invocation of the checkstyle plugin per module. Maybe we can just call the simple checkstyle.xml from the parent module of recipes and of contrib, but I did not try Enrico > Best, > tison. > > > Enrico Olivelli <eolive...@gmail.com> 于2019年8月18日周日 上午3:36写道： > > > We have merged this useful work. > > > > Now we have to merge the two checkstyle files/plugin executions into only > > one. > > > > We have one project wide configuration (all projects, contrib...) that > > check for '@author' and the new one that is a full checkstyle run. > > > > Tison, do you want to complete the work? > > > > Thank you > > > > Enrico > > > > > > > > Il dom 11 ago 2019, 13:05 Zili Chen <wander4...@gmail.com> ha scritto: > > > > > Hi, > > > > > > A quick update on this thread. > > > > > > ZOOKEEPER-3475 a.k.a GH-1049[1] has been sent for review. > > > It contains commits enabling checkstyle configuration on > > > zookeeper-server module per package, as described above. > > > > > > Best, > > > tison. > > > > > > [1] https://github.com/apache/zookeeper/pull/1049 > > > > > > > > > Enrico Olivelli <eolive...@gmail.com> 于2019年8月9日周五 下午11:13写道： > > > > > > > Il giorno ven 9 ago 2019 alle ore 16:45 Zili Chen < > > wander4...@gmail.com> > > > > ha > > > > scritto: > > > > > > > > > Hi Enrico, > > > > > > > > > > It happens my original schedule on this weekend canceled so that > > > > > I can make progress on this thread the next two days. > > > > > > > > > > From my side I would prefer multiple self-contained commits(see > > > > > below) when sending pull requests. It would helps reviews and we > > > > > can always squash and merge them. > > > > > > > > > > > > > Great. > > > > > > > > Looking forward to your patches ! > > > > > > > > I am trying to help reviews as much as possible so that patches that > > are > > > > ready will be committed as soon as possible and they won't need a > > rebase > > > > > > > > I hope we don't annoy to much back ports > > > > > > > > Thank you > > > > > > > > Enrico > > > > > > > > > > > > > > > > > > > > > > For self-contained commit, I would first enable the configuration > > > > > while suppress for all packages under zookeeper-server, and then > > > > > remove the suppression and do the formatting job. This, as > > > > > consequence, enables every commit to be applied individually and > > > > > any combination should pass CI tests. > > > > > > > > > > Here is all of packages under zookeeper-server module. > > > > > > > > > > 58 org/apache/zookeeper > > > > > 1 org/apache/zookeeper/admin > > > > > 31 org/apache/zookeeper/cli > > > > > 7 org/apache/zookeeper/client > > > > > 42 org/apache/zookeeper/common > > > > > 3 org/apache/zookeeper/jmx > > > > > 9 org/apache/zookeeper/metrics > > > > > 3 org/apache/zookeeper/metrics/impl > > > > > 104 org/apache/zookeeper/server > > > > > 15 org/apache/zookeeper/server/admin > > > > > 13 org/apache/zookeeper/server/auth > > > > > 19 org/apache/zookeeper/server/command > > > > > 9 org/apache/zookeeper/server/metric > > > > > 16 org/apache/zookeeper/server/persistence > > > > > 108 org/apache/zookeeper/server/quorum > > > > > 17 org/apache/zookeeper/server/quorum/auth > > > > > 3 org/apache/zookeeper/server/quorum/flexible > > > > > 22 org/apache/zookeeper/server/util > > > > > 16 org/apache/zookeeper/server/watch > > > > > 124 org/apache/zookeeper/test > > > > > 3 org/apache/zookeeper/util > > > > > 1 org/apache/zookeeper/version/util > > > > > > > > > > (reproduce by run > > > > > > > > > > $ find . -exec ls -dl \{\} \; | awk '{print $9}' | grep \\.java | > > perl > > > > -ne > > > > > 'if (/\/(org.*)\//) { print $1 . "\n" }' | sort | uniq -c > > > > > > > > > > under zookeeper-server/src/) > > > > > > > > > > Best, > > > > > tison. > > > > > > > > > > > > > > > Enrico Olivelli <eolive...@gmail.com> 于2019年8月9日周五 下午7:56写道： > > > > > > > > > > > It is better that we preserve variable names, method names, class > > > names > > > > > > because we could fall into bad troubles in case of cherry picks. > > > > > > > > > > > > We can comment out the rules that would force us to break such > kind > > > of > > > > > > 'compatibility' > > > > > > > > > > > > Fangmin and Micheal, you are the maintainers of two major forks, > > > > > > do you prefer one single commit or multiple commits per group of > > > > > packages? > > > > > > > > > > > > > > > > > > Enrico > > > > > > > > > > > > Il ven 9 ago 2019, 05:52 Michael Han <h...@apache.org> ha > scritto: > > > > > > > > > > > > > hi tison, that sounds good to me, thanks > > > > > > > > > > > > > > On Thu, Aug 8, 2019 at 6:40 PM Zili Chen <wander4...@gmail.com > > > > > > wrote: > > > > > > > > > > > > > > > Hi Michael, > > > > > > > > > > > > > > > > After thinking about it, I think enable checkstyle > > configuration > > > > > > > > is only about formatting. And I would make sure that there is > > no > > > > > > > > refactor. And forked repo maintainers can rebase their work > on > > > the > > > > > > > > new master just by manually applying patches. If the forked > > repo > > > > > > > > has been quite diverged from master of apache/zookeeper, a > > better > > > > > > > > way for syncing is to cherry-pick commits from > apache/zookeeper > > > and > > > > > > > > just ignore this checkstyle stuff. > > > > > > > > > > > > > > > > Best, > > > > > > > > tison. > > > > > > > > > > > > > > > > > > > > > > > > Zili Chen <wander4...@gmail.com> 于2019年8月8日周四 上午9:20写道： > > > > > > > > > > > > > > > > > @Michael > > > > > > > > > > > > > > > > > > FYI, it is possible by properly setting and updating > > > suppressing > > > > > > rules. > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > tison. > > > > > > > > > > > > > > > > > > > > > > > > > > > Michael Han <h...@apache.org> 于2019年8月8日周四 上午9:12写道： > > > > > > > > > > > > > > > > > >> I think a good approach is doing this incrementally. Doing > > > this > > > > > all > > > > > > at > > > > > > > > >> once > > > > > > > > >> over entire server code base will make life much harder > for > > > > those > > > > > > who > > > > > > > > >> maintain their own ZooKeeper forks and has internal > patches, > > > and > > > > > the > > > > > > > > >> change > > > > > > > > >> itself will be impossible to review (even though most > > changes > > > > are > > > > > > > > >> mechanical, we still need review the change). > > > > > > > > >> > > > > > > > > >> On Mon, Aug 5, 2019 at 10:57 PM Zili Chen < > > > wander4...@gmail.com > > > > > > > > > > > > wrote: > > > > > > > > >> > > > > > > > > >> > >I see some javadoc related issues in your gist, > > > > > > > > >> > >didn't we disable that rule ? JavadocType > > > > > > > > >> > > > > > > > > > >> > The failures reported are "Missing javadoc", and > > > > > > > > >> > what we disabled is "Empty javadoc". Fair enough to > > > > > > > > >> > disable "Missing javadoc" for now and filed into > > > > > > > > >> > ZOOKEEPER-3469 since a empty javadoc is also a > > > > > > > > >> > placeholder. > > > > > > > > >> > > > > > > > > > >> > >Are you using some automatic tool ? > > > > > > > > >> > > > > > > > > > >> > Yes, IntelliJ helps a lot. > > > > > > > > >> > Best, > > > > > > > > >> > tison. > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > Enrico Olivelli <eolive...@gmail.com> 于2019年8月6日周二 > > > 下午1:13写道： > > > > > > > > >> > > > > > > > > > >> > > Il giorno mar 6 ago 2019 alle ore 03:51 Zili Chen < > > > > > > > > >> wander4...@gmail.com> > > > > > > > > >> > > ha > > > > > > > > >> > > scritto: > > > > > > > > >> > > > > > > > > > > >> > > > Hi Enrico, > > > > > > > > >> > > > > > > > > > > > >> > > > Thanks for your participant! > > > > > > > > >> > > > > > > > > > > > >> > > > Running after turn on checkstyle on zookeeper-server > > > > > > > > >> > > > it reports about 9000 checkstyle failures(see > > > > > failures.txt[1]) > > > > > > > > >> > > > among 596 files(see files.txt[1]). Most of them > > > > > > > > >> > > > are related to whitespace and import. > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > I see some javadoc related issues in your gist, > > > > > > > > >> > > didn't we disable that rule ? JavadocType > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > I think I can handle it with one or two whole days > but > > > > > > > > >> > > > I'm afraid that I have no spare time until next > > > > > > weekend(08.17). > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > Are you using some automatic tool ? > > > > > > > > >> > > In the past few months in my company I have been > working > > > in > > > > > > adding > > > > > > > > >> > > checkstyle to other projects and > > > > > > > > >> > > we used Apache NetBeans to fix up the code, I guess > > > IntelliJ > > > > > can > > > > > > > do > > > > > > > > >> the > > > > > > > > >> > > same. > > > > > > > > >> > > > > > > > > > > >> > > Enrico > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > >> > > > Also I would prefer make it into one big PR and > > separate > > > > > > > > >> > > > commits per file, i.e., resolving failures per file > > for > > > > > > > > >> > > > smoothly review. > > > > > > > > >> > > > > > > > > > > > >> > > > Best, > > > > > > > > >> > > > tison. > > > > > > > > >> > > > > > > > > > > > >> > > > [1] > > > > > > > > >> > > > > https://gist.github.com/TisonKun/ba69524defbf3e9d4c99eaf2de338e5a > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > Zili Chen <wander4...@gmail.com> 于2019年8月6日周二 > > 上午9:43写道： > > > > > > > > >> > > > > > > > > > > > >> > > > > Hi Enrico, > > > > > > > > >> > > > > > > > > > > > > >> > > > > Thanks for your participant! > > > > > > > > >> > > > > > > > > > > > > >> > > > > Running after turn on checkstyle on > zookeeper-server > > > > > > > > >> > > > > it reports about 9000 checkstyle failures(see > > out.txt) > > > > > > > > >> > > > > among 596 files(see conflicts.txt). Most of them > > > > > > > > >> > > > > are related to whitespace and import. > > > > > > > > >> > > > > > > > > > > > > >> > > > > I think I can handle it with one or two whole days > > but > > > > > > > > >> > > > > I'm afraid that I have no spare time until next > > > > > > > weekend(08.17). > > > > > > > > >> > > > > > > > > > > > > >> > > > > Also I would prefer make it into one big PR and > > > separate > > > > > > > > >> > > > > commits per file, i.e., resolving failures per > file > > > for > > > > > > > > >> > > > > smoothly review. > > > > > > > > >> > > > > > > > > > > > > >> > > > > Best, > > > > > > > > >> > > > > tison. > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > Enrico Olivelli <eolive...@gmail.com> > 于2019年8月5日周一 > > > > > > 下午9:37写道： > > > > > > > > >> > > > > > > > > > > > > >> > > > >> Il giorno lun 5 ago 2019 alle ore 14:03 Zili > Chen < > > > > > > > > >> > > wander4...@gmail.com > > > > > > > > >> > > > > > > > > > > > > >> > > > >> ha > > > > > > > > >> > > > >> scritto: > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > Hi zookeepers, > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > Recently I've sent a pr[1] on enable checkstyle > > > > > > > configuration > > > > > > > > >> on > > > > > > > > >> > > > >> > zookeeper-promethus-metrics, Enrico(committer) > > and > > > > > > > > >> > > Dinesh（contributor) > > > > > > > > >> > > > >> > kindly reviewed it but we still need another > look > > > > from > > > > > > > > >> committer > > > > > > > > >> > as > > > > > > > > >> > > > >> > process required. Someone of you has spare time > > to > > > > > have a > > > > > > > > look? > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> Tison, > > > > > > > > >> > > > >> Norbert approved the PR and I have merged it. > > > > > > > > >> > > > >> We have already begun this check style work. > > > > > > > > >> > > > >> Thank you so much to you and Mao Ling for these > > > > efforts. > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > Besides, as ZOOKEEPER-3431 described, we're > going > > > to > > > > > > enable > > > > > > > > >> this > > > > > > > > >> > > > >> > configuration on zookeeper-server as following. > > > This > > > > is > > > > > > > > really > > > > > > > > >> a > > > > > > > > >> > > major > > > > > > > > >> > > > >> > package which contains a lot of codes involved > by > > > > multi > > > > > > > prs. > > > > > > > > >> I'd > > > > > > > > >> > > like > > > > > > > > >> > > > to > > > > > > > > >> > > > >> > hear from the community about how to proceed. > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> I think that if we do it quickly there will be as > > few > > > > > > > troubles > > > > > > > > as > > > > > > > > >> > > > >> possible. > > > > > > > > >> > > > >> I am available for review, just ping me. > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > (From where I stand, enable on the whole > package > > at > > > > > once > > > > > > > > >> > > > >> > could solve the problem at once, but it would > > > > conflict > > > > > > > quite > > > > > > > > a > > > > > > > > >> lot > > > > > > > > >> > > > >> > ongoing prs. On the other hand, sending the pr > > > > without > > > > > > > timely > > > > > > > > >> > > reviews > > > > > > > > >> > > > >> > also causes a burden to rebase it on the master > > and > > > > > solve > > > > > > > > >> conflict > > > > > > > > >> > > > >> > again and again(since it likely conflicts with > a > > > > merged > > > > > > > pr)) > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> Did you check how many violations we have con > > > > > > > zookeeper-server > > > > > > > > ? > > > > > > > > >> > > > >> How much big is the work ? > > > > > > > > >> > > > >> Do you have an estimate ? > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> We could take into consideration to have only one > > big > > > > PR. > > > > > > > > >> > > > >> The tradeoff is that it will be very difficult to > > > > > perform a > > > > > > > > >> review, > > > > > > > > >> > we > > > > > > > > >> > > > >> will > > > > > > > > >> > > > >> need more eyes on the diff > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> Enrico > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > Best, > > > > > > > > >> > > > >> > tison. > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > [1] > > https://github.com/apache/zookeeper/pull/1028 > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
