Consistent code style is crucial for a large project. In order to make Pulsar better, I believe we shouldn't be afraid of "risks". By introducing Spotless, we can permanently resolve the issue of inconsistent code style and ensure that all contributors adhere to this rule moving forward. If we don't make these changes now, we might have to deal with changes in over 3000 files today and potentially over 5000 files tomorrow.
Thanks, sinan Dave Fisher <wave4d...@comcast.net> 于2023年9月1日周五 12:19写道： > -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. > >>> > >
