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