You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Zixuan Liu <no...@gmail.com> on 2023/09/01 04:04:33 UTC
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
This idea looks good to me, but we need to format all codebase to
avoid conflicts during cherry picking.
+1
Asaf Mesika <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
> >
> > > Tison,
> > >
> > > Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
> > > > > > >> Date: Friday, June 30, 2023 at 6:45 AM
> > > > > > >> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
> > > > > > >> Date: Friday, June 30, 2023 at 3:58 AM
> > > > > > >> To: Dev <de...@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.
> >
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by Asaf Mesika <as...@gmail.com>.
Hang, we can do this piecemeal - Folder by folder, so we can throttle the
amount of PR to make them easy to review.
To make it sustainable, perhaps we can do this only on master?
Regarding git blame - you have the same issue when you view a file that has
been heavily modified in one commit, right?
It shouldn't be an issue - Show History in IntelliJ, pick the one commit
before. ,right click -> annotate - boom you have the git blame for the
previous commit.
I agree we need a reliable Pulsar, but I think a program doing the
modification is likely to produce 0 bugs - it's not human based.
Also, we are afraid to make changes, Pulsar will not get better.
Remember Pulsar is very early stage adoption. Now is *exactly* the time to
do it.
Also keep in mind: reliability comes when code is readable.
On Tue, Sep 5, 2023 at 5:56 AM Hang Chen <ch...@apache.org> 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 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
>
> 徐昀泽 <xy...@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 <as...@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 <wa...@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 <li...@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 <wa...@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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
> > >>>>>>>
> > >>>>>>>> Tison,
> > >>>>>>>>
> > >>>>>>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
> > >>>>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
> > >>>>>>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
> > >>>>>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
> > >>>>>>>>>>>>> To: Dev <de...@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.
> > >>>>>>>
> > >>>>
> > >>>>
> > >>
> > >>
> >
>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by Hang Chen <ch...@apache.org>.
>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
徐昀泽 <xy...@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 <as...@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 <wa...@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 <li...@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 <wa...@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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
> >>>>>>>
> >>>>>>>> Tison,
> >>>>>>>>
> >>>>>>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
> >>>>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
> >>>>>>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
> >>>>>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
> >>>>>>>>>>>>> To: Dev <de...@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.
> >>>>>>>
> >>>>
> >>>>
> >>
> >>
>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by 徐昀泽 <xy...@gmail.com>.
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 <as...@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 <wa...@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 <li...@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 <wa...@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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
>>>>>>>
>>>>>>>> Tison,
>>>>>>>>
>>>>>>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
>>>>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
>>>>>>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
>>>>>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
>>>>>>>>>>>>> To: Dev <de...@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.
>>>>>>>
>>>>
>>>>
>>
>>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by Asaf Mesika <as...@gmail.com>.
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 <wa...@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 <li...@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 <wa...@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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
> >>>>>
> >>>>>> Tison,
> >>>>>>
> >>>>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
> >>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
> >>>>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
> >>>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
> >>>>>>>>>>> To: Dev <de...@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.
> >>>>>
> >>
> >>
>
>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by Dave Fisher <wa...@comcast.net>.
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 <li...@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 <wa...@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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
>>>>>
>>>>>> Tison,
>>>>>>
>>>>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
>>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
>>>>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
>>>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
>>>>>>>>>>> To: Dev <de...@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.
>>>>>
>>
>>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by SiNan Liu <li...@gmail.com>.
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 <wa...@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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
> >>>
> >>>> Tison,
> >>>>
> >>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
> >>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
> >>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
> >>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
> >>>>>>>>> To: Dev <de...@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.
> >>>
>
>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Posted by Dave Fisher <wa...@comcast.net>.
-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 <no...@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 <as...@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 <wa...@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 <eo...@gmail.com>于2023年8月16日 周三00:50写道：
>>>
>>>> Tison,
>>>>
>>>> Il Mar 15 Ago 2023, 16:56 tison <wa...@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 <pe...@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 <wa...@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 <wa...@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 <ki...@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 <ki...@teal.tools>
>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM
>>>>>>>>> To: dev@pulsar.apache.org <de...@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 <wa...@gmail.com>
>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM
>>>>>>>>> To: Dev <de...@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.
>>>