>While I can agree that a consistent style can help I don’t agree that it is >necessary. If the compiler understands the code then IMO we are good.
I agree with Dave's idea. My concern is how much value this change will bring to Pulsar. What's more, it will bring other burdens, such as PR review, PR cherry-pick, git blames. The main goal of Pulsar is to improve the reliability. -1 for this change. Best, Hang 徐昀泽 <xyzinfern...@gmail.com> 于2023年9月4日周一 19:24写道： > > Well, I’m just back to this thread. > > Now I’m +1 to this extremely huge change, but to be more friendly to > developers, > we should document the workarounds for the git blame issue. And we should > apply > the spotless tool to every active branches. > > > On Sep 3, 2023, at 19:43, Asaf Mesika <asaf.mes...@gmail.com> wrote: > > > > I couldn't stress how much I oppose the sentence "If the compiler > > understands the code then IMO we are good." > > > > Sinan is right: This project needs to take calculated risks in order to > > move forward to be better. > > Yes I agree prioritizing is super important, since Pulsar has *so many* > > fronts to be better at. > > > > We need more people on this thread, to get a wide angle on this IMO. > > > > > > On Sat, Sep 2, 2023 at 7:27 AM Dave Fisher <wave4d...@comcast.net> wrote: > > > >> While I can agree that a consistent style can help I don’t agree that it > >> is necessary. If the compiler understands the code then IMO we are good. > >> > >> I am a bit of a dinosaur since I have keypunched code on cards in my > >> career. I’ve played with writing interpreters and specialized languages. > >> > >> But I’m -0 and if the project prefers strict code style then that is fine > >> too! > >> > >> If anyone agrees with me know that part of consensus building is to > >> provide opinions and accept divergent results. > >> > >> Best, > >> Dave > >> > >> PS. If tisun wants to put on their superhero cape and convert the code > >> base then let’s acknowledge that AND let’s consider all of the PRs that are > >> now effectively closed. > >> > >> Sent from my iPhone > >> > >>> On Sep 1, 2023, at 8:57 PM, SiNan Liu <liusinan1...@gmail.com> wrote: > >>> > >>> 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. > >>>>>>> > >>>> > >>>> > >> > >> >
