I would wait to make changes on a branch immediately after the next patch releases.
Best, Dave Sent from my iPhone > On Jun 29, 2023, at 8:52 PM, tison <wander4...@gmail.com> wrote: > > `git blame` can set a start commit [1] so you can actually blame before the > format commit. It's integrated with IDEA[2] and GitHub Web UI[3][4] also. > > Best, > tison. > > [1] > https://stackoverflow.com/questions/5098256/how-can-i-view-prior-commits-with-git-blame > [2] > https://www.jetbrains.com/help/idea/viewing-changes-information.html#annotating-previous-versions > [3] > https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view > [4] https://github.com/refined-github/refined-github/issues/2175 > > > Yunze Xu <x...@apache.org> 于2023年6月30日周五 11:38写道： > >> One thing I’m concerned about is that the code reformatting could bring >> very >> wide changes and it’s very unfriendly to the `git blame` command. >> >> The check style plugin was not applied to all modules as well. There were >> many >> PRs to apply the check style plugin for different modules. Take my PR [1] >> as >> example, which applies the check style plugin for the pulsar-client >> module. There >> are over 2000 lines changes. It’s hard to review and it also brings a >> breaking >> change that was missed by the reviewers. You can see a public class >> `Murmur3_32Hash` >> was renamed with `Murmur3Hash32`. >> >> [1] https://github.com/apache/pulsar/pull/13940 >> >> Thanks, >> Yunze >> >>> On Fri, Jun 30, 2023 at 10:48 AM Joo Hyuk Kim <beansk...@gmail.com> wrote: >>> >>>> Spotless has an IDEA plugin, but it's for Gradle task only. >>> >>> I saw this one also 🥲. >>> >>>> [1] can be used >>> >>> Nice, IMO having at least one will suffice 👍🏻. >>> Thanks, >>> joohyuk >>> >>>> On Fri, Jun 30, 2023 at 11:43 AM tison <wander4...@gmail.com> wrote: >>> >>>>> IDE >>>> >>>> If using the palantir style, [1] can be used. Spotless has an IDEA >> plugin >>>> but it's for Gradle task only. >>>> >>>> Best, >>>> tison. >>>> >>>> [1] https://plugins.jetbrains.com/plugin/13180-palantir-java-format >>>> >>>> >>>> Joo Hyuk Kim <beansk...@gmail.com> 于2023年6月30日周五 10:38写道： >>>> >>>>> Sounds great. >>>>> >>>>> Another thing is about IDE. >>>>> Pulsar site sort of directs contributors to use IntelliJ as shown in >>>>> https://pulsar.apache.org/contribute/setup-ide/. >>>>> It would be nice if there was IDE support (auto-format, or >> shortkey-base) >>>>> and we can add it to the setup-ide guide also. >>>>> Do you know any? Or I can do some research if you don't. >>>>> >>>>> Thanks, >>>>> joohyuk >>>>> >>>>> On Fri, Jun 30, 2023 at 10:58 AM tison <wander4...@gmail.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> I'd like to propose applying a consistent code style (especially >>>>> whitespace >>>>>> and indent) with an autotool Spotless. >>>>>> >>>>>> // Background >>>>>> >>>>>> Over and over we argue contributors reformat their patch manually >> for >>>>>> checkstyle violations, or even whitespace changes that are not >> detected >>>>> by >>>>>> checkstyle. [1] >>>>>> >>>>>> A common reason is that such style-only changes increase the >> burden to >>>> do >>>>>> cherry-pick if a later bug fix is made around the code while we >> often >>>> do >>>>>> not pick the style change barely or even it's coupled with an >>>> unpickable >>>>>> commit. >>>>>> >>>>>> CheckStyle helps in some cases while we don't have a strong rule >> set >>>> nor >>>>>> even apply it to tests (porting bug fix actually include the test). >>>>>> >>>>>> Manually fixing style violations can be boring and even annoying. >>>> That's >>>>>> why it's effectively "suppressed" in mind. >>>>>> >>>>>> An autotool to do the formatting work can help and here comes >>>>> Spotless[2]. >>>>>> Flink and Curator use it and Flink actually migrated from >> CheckStyle to >>>>>> Spotless for its more strict consistent rule set and oneliner >> format. >>>> See >>>>>> their original discussion for the context[3]. >>>>>> >>>>>> // Proprosal >>>>>> >>>>>> Using Spotless as the auto-formatting tool in all around >> apache/pulsar. >>>>>> Since it's an auto tool I can cover the task solely. >>>>>> >>>>>> // Concerns >>>>>> >>>>>> 1. Won't it be yet another noise to the codebase? >>>>>> >>>>>> Yes and no. I suggest we pick this change to all maintained >> versions so >>>>>> that all of them align with the consistent style. Then from now >> on, no >>>>> more >>>>>> style conflict. >>>>>> >>>>>> Flink used the same strategy[4] and even we can do it starting from >>>> 2.10 >>>>> as >>>>>> Lari proposed to apply commits from the least recent maintained >>>> version. >>>>> I >>>>>> understand the maintained versions are: 2.10, 2.11, 3.0, and >> master. >>>>>> >>>>>> 2. Can it cover all the rule sets we use in CheckStyle now? >>>>>> >>>>>> Let's say we almost don't care what the style is but it's >> consistent >>>> and >>>>>> "not awful". >>>>>> >>>>>> The default Google style takes line length = 80 which can be a >>>>>> disappointment so in Curator I use the palantir style[5] which >> takes >>>> line >>>>>> length = 120 which should keep consistent with current settings. >>>>>> >>>>>> A downside is that Spotless Maven plugin cannot detect "forbidden >>>>> imports" >>>>>> where we can still use CheckStyle[6] - this is a low-traffic path >> and >>>> it >>>>>> should be fine. >>>>>> >>>>>> // Implementation >>>>>> >>>>>> 1. Introduce Spotless in the project and apply it around the >> codebase, >>>>> for >>>>>> all maintained versions. >>>>>> 2. Remove the then redundant CheckStyle rules, while retaining the >>>>>> forbidden imports part as it's still useful. >>>>>> >>>>>> Looking forward to your feedback! >>>>>> >>>>>> Best, >>>>>> tison. >>>>>> >>>>>> [1] >>>>>> >>>>>> >>>>> >>>> >> https://github.com/apache/pulsar/pull/20642/files#diff-cb9b09b67f54fccdd5155dbbcedd50970ee93d9ee85ade1e6b6984cab64dab5d >>>>>> [2] https://github.com/diffplug/spotless >>>>>> [3] >> https://lists.apache.org/thread/3kjkcz9gj6f8j477d1t3gnbkl61hsb7z >>>>>> [4] >> https://lists.apache.org/thread/nxdm0pg84q913w0kxszm502myqcg3db0 >>>>>> [5] https://github.com/palantir/palantir-java-format >>>>>> [6] https://issues.apache.org/jira/browse/FLINK-32154 >>>>>> >>>>> >>>> >>
