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