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
