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) <[email protected]> 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 <[email protected]> 于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) <[email protected]> > wrote: > > > > > +1 > > > > > > Andrew Purtell <[email protected]> 于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 <[email protected]> > > > > 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) < > [email protected] > > > > > > > > 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 <[email protected]>于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) < > > > [email protected]> > > > > >> > 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 <[email protected]> > > > 于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 < > > > > >> [email protected]> > > > > >> > > > 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) <[email protected]> 于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
