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