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