-0. I think it will introduce too much change. We have classes that are quite large and having to fix code style after making a small correction seems like a waste of volunteer energy.
Best, Dave Sent from my iPhone > On Aug 31, 2023, at 9:05 PM, Zixuan Liu <node...@gmail.com> wrote: > > This idea looks good to me, but we need to format all codebase to > avoid conflicts during cherry picking. > > +1 > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年8月31日周四 20:39写道： >> >> Opentelemetry-java employs both spotless for coding style >> You run "./gradlew spotlessCheck" and it shows the problems. >> You run "./gradlew spotlessApply" to automatically fix it. >> >> It also uses errorprone to detect bugs. >> >> I wonder if we can run it only in GitHub PRs, so we can instruct it to run >> only on files you have changed / added. This is we "throttle" the style >> through files touched. >> >> >> >>> On Tue, Aug 15, 2023 at 8:31 PM tison <wander4...@gmail.com> wrote: >>> >>> These have been discussed that - >>> >>> 1. Cherrypick: we will reformat for all maintained branches. >>> 2. Commit noise: metadata to filter out during blame. >>> 3. PR rebased: invincible, while we don't have a large backlog or active >>> large pending PR. >>> >>> But if our critical mass agree this is not a good tradeoff, there is no >>> issue to "resolve". >>> >>> Enrico Olivelli <eolive...@gmail.com>于2023年8月16日 周三00:50写道： >>> >>>> Tison, >>>> >>>> Il Mar 15 Ago 2023, 16:56 tison <wander4...@gmail.com> ha scritto: >>>> >>>>> A demostration for change set - >>>>> https://github.com/apache/pulsar/pull/20974 >>>> >>>> >>>> >>>> While I think it is great to start with Spotless for little projects or >>>> when you start from scratch, >>>> appling a patch that changes 3.000+ files will make it very hard to >>> rebase >>>> all the pending PRs and also to cherry pick changes to older branches >>> that >>>> we support. >>>> >>>> I think that this is not a good option for Pulsar at this stage. >>>> >>>> Maybe if we had a configuration that doesn't change so many files.... >>>> >>>> Enrico >>>> >>>>> >>>>> >>>>> If we go forward this direction, it should be picked to branch-3.0 and >>>>> branch-3.1. Earlier versions can be ported on demand and I'm glad to >>>>> volunteer doing that. >>>>> >>>>> Best, >>>>> tison. >>>>> >>>>> >>>>> PengHui Li <peng...@apache.org> 于2023年7月10日周一 10:00写道： >>>>> >>>>>> My concern is how much value it will add. >>>>>> From my experience, it's fine. The code style >>>>>> is not consistent but doesn't affect my code reading >>>>>> and writing much. But it might introduce risks and we >>>>>> need to pay much effort to the code review. >>>>>> >>>>>> Regards, >>>>>> Penghui >>>>>> >>>>>> On Wed, Jul 5, 2023 at 7:39 PM tison <wander4...@gmail.com> wrote: >>>>>> >>>>>>> ... which seems a GitHub only extension - >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ >>>>>>> >>>>>>> Best, >>>>>>> tison. >>>>>>> >>>>>>> >>>>>>> tison <wander4...@gmail.com> 于2023年7月5日周三 19:38写道： >>>>>>> >>>>>>>> For the git blame issue, I found also this practice in >>>> StreamPark[1]. >>>>>>>> >>>>>>>> cc @Yunze. >>>>>>>> >>>>>>>> Best, >>>>>>>> tison. >>>>>>>> >>>>>>>> [1] >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> https://github.com/apache/incubator-streampark/blob/cac931ae289e0753892279336e1c4e70e5f7d7c6/.git-blame-ignore-revs >>>>>>>> >>>>>>>> >>>>>>>> Kiryl Valkovich <kiryl_valkovich@teal.tools> 于2023年6月30日周五 >>>> 13:03写道： >>>>>>>> >>>>>>>>> My mistake. It looks that for some reason Spotless supports >>>>>>> .editorconfig >>>>>>>>> only for ktlint. >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> Kiryl >>>>>>>>> >>>>>>>>> From: Kiryl Valkovich <kiryl_valkovich@teal.tools> >>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM >>>>>>>>> To: dev@pulsar.apache.org <dev@pulsar.apache.org> >>>>>>>>> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) >>> and >>>>>>>>> autotools >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> tison, are you going to use .editorconfig for specifying indent >>>>> rules? >>>>>>>>> >>>>>>>>> https://editorconfig.org/ >>>>>>>>> >>>>>>>>> It looks like Spotless supports it. >>>>>>>>> https://github.com/diffplug/spotless/issues/734 >>>>>>>>> >>>>>>>>> Flink and many other ASF projects use it. >>>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig >>>>>>>>> >>>> https://github.com/search?q=org%3Aapache%20.editorconfig&type=code >>>>>>>>> >>>>>>>>> Unlike Spotless, the .editorconfig works out of the box in most >>> of >>>>> the >>>>>>>>> modern code editors. >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> Kiryl >>>>>>>>> >>>>>>>>> From: tison <wander4...@gmail.com> >>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM >>>>>>>>> To: Dev <dev@pulsar.apache.org> >>>>>>>>> Subject: [DISCUSS] Consistent code style (esp. ws/indent) and >>>>>> autotools >>>>>>>>> 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 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> -- >>> Best, >>> tison. >>>
