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