+1 for Duo's bump here -- let's bring this home! On Tue, Apr 26, 2022 at 4:08 PM 张铎(Duo Zhang) <palomino...@gmail.com> wrote:
> Just a reminder that some of us have almost reached a consensus on the > final formatter config file. > > PTAL about the discussion and also the final style in this PR > https://github.com/apache/hbase/pull/4312 > > We have also talked about whether we should just use some well known styles > such as google java format. > > So please reply here if you have any thoughts on this, i.e, which style do > you prefer more, the current eclipse formatter, or google java format, or > some other styles. > > Thanks a lot! > > 张铎(Duo Zhang) <palomino...@gmail.com> 于2022年4月13日周三 07:48写道： > > > Some heads up, the current state on the big reformat can be reviewed here > > > > https://github.com/apache/hbase/pull/4312 > > > > Shout if you have any questions about the formatter, we could try to > tweak > > the eclipse formatter to see if it could work as expected. > > > > Thanks. > > > > Andrew Purtell <apurt...@apache.org> 于2022年4月3日周日 02:34写道： > > > >> I think we have consensus to move forward with application of the PRs, > so > >> let's fine tune the formatting configuration, and then regenerate and > >> apply > >> them. > >> > >> > >> On Thu, Mar 31, 2022 at 11:03 PM 张铎(Duo Zhang) <palomino...@gmail.com> > >> wrote: > >> > >> > FWIW, you could do a 'git reset --hard' to the commit before the big > >> > reformat and do a 'git blame' again on command line. > >> > > >> > Nick Dimiduk <ndimi...@apache.org> 于2022年4月1日周五 13:58写道： > >> > > >> > > I noticed that GitHub’s blame UI has a “show before this commit” > >> feature, > >> > > which allowed me to look at the blame info before the reformat was > >> > applied. > >> > > I assume the same is possible on the command line. > >> > > > >> > > On Fri, Apr 1, 2022 at 04:06 张铎(Duo Zhang) <palomino...@gmail.com> > >> > wrote: > >> > > > >> > > > +1 > >> > > > > >> > > > Andrew Purtell <apurt...@apache.org> 于2022年4月1日周五 06:18写道： > >> > > > > >> > > > > Now that I understand what 'ratchetFrom' in the plugin > >> configuration > >> > > was > >> > > > > doing, I have removed it, and resubmitted the PRs. They are all > >> > > > ridiculous > >> > > > > and expected. Just about every file in the source is touched. On > >> > master > >> > > > > branch 4679 files are modified out of 5507 (excluding .git/). > >> There > >> > are > >> > > > > similar results for other branches. > >> > > > > > >> > > > > I think we should first spot check the PRs to determine if > >> spotless > >> > > > > configuration should be fine tuned. Then once it is acceptable > the > >> > PRs > >> > > > can > >> > > > > be updated with a re-application. Merging them applies a one > shot > >> > > > > reformatting to all live branches. This is desirable so that > going > >> > > > forward > >> > > > > it will be easier for reviewers and committers to manage PRs and > >> > > commits > >> > > > to > >> > > > > multiple branches. Put another way, if we don't do it, merge > >> > conflicts > >> > > > are > >> > > > > more likely. > >> > > > > > >> > > > > By adopting spotless and a particular configuration as our > coding > >> > > > standard, > >> > > > > and by opting in to automatic application of that policy as part > >> of > >> > our > >> > > > > patch submission process, we also agree that a discontinuity in > >> > history > >> > > > > (i.e. 'git blame' will be less useful prior to the application > of > >> the > >> > > > > reformatting than afterward) is an acceptable trade off. If this > >> is > >> > > true > >> > > > we > >> > > > > can move forward. If not it needs more discussion. > >> > > > > > >> > > > > > >> > > > > On Mon, Mar 28, 2022 at 7:08 PM Andrew Purtell < > >> apurt...@apache.org> > >> > > > > wrote: > >> > > > > > >> > > > > > Yes, I figure we should run spotless on all live branch-2x > >> branches > >> > > to > >> > > > > get > >> > > > > > a baseline for future contributions and backports. > >> > > > > > > >> > > > > > On Mon, Mar 28, 2022 at 6:50 PM 张铎(Duo Zhang) < > >> > palomino...@gmail.com > >> > > > > >> > > > > > wrote: > >> > > > > > > >> > > > > >> I’m natural on this. We could remove the ratchetFrom config > and > >> > do a > >> > > > > full > >> > > > > >> reformat on all branches. The problem is git blame will be > >> > useless… > >> > > > > >> > >> > > > > >> But anyway, if the file has been touched later, git blame > will > >> be > >> > > > broken > >> > > > > >> too. > >> > > > > >> > >> > > > > >> I saw Andrew has already opened a PR. Let’s get it done. > >> > > > > >> > >> > > > > >> Thanks. > >> > > > > >> > >> > > > > >> Bryan Beaudreault <bbeaudrea...@hubspot.com > >> .invalid>于2022年3月29日 > >> > > > > >> 周二00:55写道： > >> > > > > >> > >> > > > > >> > Thanks for your work here Duo! > >> > > > > >> > > >> > > > > >> > What should be our convention on committing unrelated > >> formatting > >> > > > > changes > >> > > > > >> > due to spotless? For example, I have a PR > >> > > > > >> > https://github.com/apache/hbase/pull/4180 open. I rebased > >> it on > >> > > > > latest > >> > > > > >> > master to pull in spotless and ran `mvn spotless:apply` as > >> > > > suggested. > >> > > > > As > >> > > > > >> > you said in the jira, it properly only applied to the > files I > >> > > > touched. > >> > > > > >> But > >> > > > > >> > some of those files I touched had lots of formatting > changes > >> > > > unrelated > >> > > > > >> to > >> > > > > >> > my changes. I imagine this will make it harder to review. > >> > > > > >> > > >> > > > > >> > I wonder if we should do a single pass of spotbugs:apply to > >> the > >> > > main > >> > > > > >> active > >> > > > > >> > branches to avoid cases like this. It would probably be a > >> big PR > >> > > but > >> > > > > >> should > >> > > > > >> > be relatively safe due to just being formatting. Maybe this > >> was > >> > > > > already > >> > > > > >> in > >> > > > > >> > your plans. > >> > > > > >> > > >> > > > > >> > On Mon, Mar 28, 2022 at 9:26 AM 张铎(Duo Zhang) < > >> > > > palomino...@gmail.com> > >> > > > > >> > wrote: > >> > > > > >> > > >> > > > > >> > > Thanks all for chimming in. HBASE-26617 has been resolved > >> and > >> > > the > >> > > > > >> patch > >> > > > > >> > has > >> > > > > >> > > been committed to branch-2.5+. > >> > > > > >> > > > >> > > > > >> > > Will start to work on HBASE-26892 to integrate spotless > >> check > >> > in > >> > > > our > >> > > > > >> pre > >> > > > > >> > > commit build. > >> > > > > >> > > > >> > > > > >> > > Bryan Beaudreault <bbeaudrea...@hubspot.com.invalid> > >> > > > 于2022年3月26日周六 > >> > > > > >> > > 02:35写道： > >> > > > > >> > > > >> > > > > >> > > > +1, agreed. It would be nice to backport to branch-2 as > >> well > >> > > so > >> > > > we > >> > > > > >> can > >> > > > > >> > > keep > >> > > > > >> > > > the branches somewhat consistent in style at least. > >> > > > > >> > > > > >> > > > > >> > > > On Fri, Mar 25, 2022 at 2:05 PM Huaxiang Sun < > >> > > > > >> huaxiang...@apache.org> > >> > > > > >> > > > wrote: > >> > > > > >> > > > > >> > > > > >> > > > > +1, a very nice and helpful feature. > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > On 2022/03/25 04:04:31 "张铎(Duo Zhang)" wrote: > >> > > > > >> > > > > > Revive. > >> > > > > >> > > > > > > >> > > > > >> > > > > > Will wait for another couple of days, if no big > >> > > objections, > >> > > > I > >> > > > > >> will > >> > > > > >> > > move > >> > > > > >> > > > > > forward to integrate spotless into our active > >> branches. > >> > > > > >> > > > > > > >> > > > > >> > > > > > Thanks. > >> > > > > >> > > > > > > >> > > > > >> > > > > > 张铎(Duo Zhang) <palomino...@gmail.com> > 于2022年3月15日周二 > >> > > > 21:17写道： > >> > > > > >> > > > > > > >> > > > > >> > > > > > > I've filed HBASE-26617 a while ago and recently I > >> > > > > implemented > >> > > > > >> a > >> > > > > >> > PR > >> > > > > >> > > > for > >> > > > > >> > > > > > > master branch. > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > https://github.com/apache/hbase/pull/4214 > >> > > > > >> > > <https://github.com/apache/hbase/pull/4214> > >> > > > > >> > > > > <https://github.com/apache/hbase/pull/4214 > >> > > > > >> > > <https://github.com/apache/hbase/pull/4214> > >> > > > > >> > > > > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > The PR is a bit large because it will also format > >> the > >> > > pom, > >> > > > > the > >> > > > > >> > > actual > >> > > > > >> > > > > > > changes are here > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > >> > > > > >> > > >> > > > > >> > >> > > > > > >> > > > > >> > > > >> > > >> > https://github.com/apache/hbase/pull/4214/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R2673 > >> > > > > >> > > < > >> > > > > >> > > >> > > > > >> > >> > > > > > >> > > > > >> > > > >> > > >> > https://github.com/apache/hbase/pull/4214/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R2673 > >> > > > > >> > > > >> > > > > >> > > > > < > >> > > > > >> > > > > >> > > > > >> > > > >> > > > > >> > > >> > > > > >> > >> > > > > > >> > > > > >> > > > >> > > >> > https://github.com/apache/hbase/pull/4214/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R2673 > >> > > > > >> > > < > >> > > > > >> > > >> > > > > >> > >> > > > > > >> > > > > >> > > > >> > > >> > https://github.com/apache/hbase/pull/4214/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R2673 > >> > > > > >> > > > >> > > > > >> > > > > > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > In the PR I make use of the eclipse formatter and > >> > import > >> > > > > order > >> > > > > >> > file > >> > > > > >> > > > to > >> > > > > >> > > > > > > format our java file. For pom, I just use most of > >> the > >> > > > > default > >> > > > > >> > > > > formatter, > >> > > > > >> > > > > > > the only exception is to expand empty element. > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > The spotless plugin support setting a ratchetFrom > >> > > option, > >> > > > > >> which > >> > > > > >> > > could > >> > > > > >> > > > > be a > >> > > > > >> > > > > > > commit or a tag. Only files changed after this > >> commit > >> > > will > >> > > > > be > >> > > > > >> > > > > formatted, so > >> > > > > >> > > > > > > we can avoid generating a very big patch when > >> running > >> > > > > >> > > spotless:apply. > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > So after we get this in, before submitting a PR, > >> you > >> > can > >> > > > > just > >> > > > > >> > type > >> > > > > >> > > > mvn > >> > > > > >> > > > > > > spotless:apply, then most of the checkstyle > issues > >> > will > >> > > be > >> > > > > >> solved > >> > > > > >> > > > > > > automatically. And we could also add a > >> spotless:check > >> > > step > >> > > > > in > >> > > > > >> our > >> > > > > >> > > pre > >> > > > > >> > > > > > > commit job, to make sure we run spotless:apply > >> before > >> > > > > >> submitting > >> > > > > >> > a > >> > > > > >> > > > PR. > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > Just tell me what you think about the above plan. > >> > > > > Suggestions > >> > > > > >> are > >> > > > > >> > > > > always > >> > > > > >> > > > > > > welcomed. > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > Thanks. > >> > > > > >> > > > > > > > >> > > > > >> > > > > > > >> > > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > >> > > > > >> > > >> > > > > >> > >> > > > > > > >> > > > > > > >> > > > > > -- > >> > > > > > Best regards, > >> > > > > > Andrew > >> > > > > > > >> > > > > > Unrest, ignorance distilled, nihilistic imbeciles - > >> > > > > > It's what we’ve earned > >> > > > > > Welcome, apocalypse, what’s taken you so long? > >> > > > > > Bring us the fitting end that we’ve been counting on > >> > > > > > - A23, Welcome, Apocalypse > >> > > > > > > >> > > > > > >> > > > > > >> > > > > -- > >> > > > > Best regards, > >> > > > > Andrew > >> > > > > > >> > > > > Unrest, ignorance distilled, nihilistic imbeciles - > >> > > > > It's what we’ve earned > >> > > > > Welcome, apocalypse, what’s taken you so long? > >> > > > > Bring us the fitting end that we’ve been counting on > >> > > > > - A23, Welcome, Apocalypse > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >> -- > >> Best regards, > >> Andrew > >> > >> Unrest, ignorance distilled, nihilistic imbeciles - > >> It's what we’ve earned > >> Welcome, apocalypse, what’s taken you so long? > >> Bring us the fitting end that we’ve been counting on > >> - A23, Welcome, Apocalypse > >> > > >
