[DISCUSS] Consistent code style (esp. ws/indent) and autotools

[tison <wa...@gmail.com>]

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[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

[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

[徐昀泽 <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

[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

[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

[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

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Zixuan Liu <no...@gmail.com>]

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

[Asaf Mesika <as...@gmail.com>]

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

[tison <wa...@gmail.com>]

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

[Enrico Olivelli <eo...@gmail.com>]

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[tison <wa...@gmail.com>]

A demostration for change set - https://github.com/apache/pulsar/pull/20974

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[PengHui Li <pe...@apache.org>]

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[tison <wa...@gmail.com>]

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[tison <wa...@gmail.com>]

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
>

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Kiryl Valkovich <ki...@teal.tools>]

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Kiryl Valkovich <ki...@teal.tools>]

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Lari Hotari <lh...@apache.org>]

Making a change to formatting is a broad change that cause more harm than benefit to the Pulsar project currently. 

We can reconsider after the maintenance strategy has been revisited.
The discussion about the maintenance strategy is in a separate thread [1].
Even with a different type of maintenance strategy, formatting changes could be just an unnecessary distraction. I'd rather put the effort on real refactoring improvements instead of spending a few months on whitespace changes.

-Lari

1 - https://lists.apache.org/thread/j6qvt45rndnvjypcmqxsfmddqt41bxjv

On 2024/03/18 09:27:57 Zixuan Liu wrote:
> Top
> 
> tison <wa...@gmail.com> 于2023年6月30日周五 09:58写道：
> 
> > 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
> >
> 

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Zixuan Liu <no...@gmail.com>]

Top

tison <wa...@gmail.com> 于2023年6月30日周五 09:58写道：

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

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Dave Fisher <wa...@comcast.net>]

I would wait to make changes on a branch immediately after the next patch releases.

Best,
Dave 

Sent from my iPhone

> On Jun 29, 2023, at 8:52 PM, tison <wa...@gmail.com> wrote:
> 
> `git blame` can set a start commit [1] so you can actually blame before the
> format commit. It's integrated with IDEA[2] and GitHub Web UI[3][4] also.
> 
> Best,
> tison.
> 
> [1]
> https://stackoverflow.com/questions/5098256/how-can-i-view-prior-commits-with-git-blame
> [2]
> https://www.jetbrains.com/help/idea/viewing-changes-information.html#annotating-previous-versions
> [3]
> https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view
> [4] https://github.com/refined-github/refined-github/issues/2175
> 
> 
> Yunze Xu <xy...@apache.org> 于2023年6月30日周五 11:38写道：
> 
>> One thing I’m concerned about is that the code reformatting could bring
>> very
>> wide changes and it’s very unfriendly to the `git blame` command.
>> 
>> The check style plugin was not applied to all modules as well. There were
>> many
>> PRs to apply the check style plugin for different modules. Take my PR [1]
>> as
>> example, which applies the check style plugin for the pulsar-client
>> module. There
>> are over 2000 lines changes. It’s hard to review and it also brings a
>> breaking
>> change that was missed by the reviewers. You can see a public class
>> `Murmur3_32Hash`
>> was renamed with `Murmur3Hash32`.
>> 
>> [1] https://github.com/apache/pulsar/pull/13940
>> 
>> Thanks,
>> Yunze
>> 
>>> On Fri, Jun 30, 2023 at 10:48 AM Joo Hyuk Kim <be...@gmail.com> wrote:
>>> 
>>>> Spotless has an IDEA plugin, but it's for Gradle task only.
>>> 
>>> I saw this one also 🥲.
>>> 
>>>> [1] can be used
>>> 
>>> Nice, IMO having at least one will suffice 👍🏻.
>>> Thanks,
>>> joohyuk
>>> 
>>>> On Fri, Jun 30, 2023 at 11:43 AM tison <wa...@gmail.com> wrote:
>>> 
>>>>> IDE
>>>> 
>>>> If using the palantir style, [1] can be used. Spotless has an IDEA
>> plugin
>>>> but it's for Gradle task only.
>>>> 
>>>> Best,
>>>> tison.
>>>> 
>>>> [1] https://plugins.jetbrains.com/plugin/13180-palantir-java-format
>>>> 
>>>> 
>>>> Joo Hyuk Kim <be...@gmail.com> 于2023年6月30日周五 10:38写道：
>>>> 
>>>>> Sounds great.
>>>>> 
>>>>> Another thing is about IDE.
>>>>> Pulsar site sort of directs contributors to use IntelliJ as shown in
>>>>> https://pulsar.apache.org/contribute/setup-ide/.
>>>>> It would be nice if there was IDE support (auto-format, or
>> shortkey-base)
>>>>> and we can add it to the setup-ide guide also.
>>>>> Do you know any? Or I can do some research if you don't.
>>>>> 
>>>>> Thanks,
>>>>> joohyuk
>>>>> 
>>>>> On Fri, Jun 30, 2023 at 10:58 AM tison <wa...@gmail.com> wrote:
>>>>> 
>>>>>> 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
>>>>>> 
>>>>> 
>>>> 
>> 

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[tison <wa...@gmail.com>]

`git blame` can set a start commit [1] so you can actually blame before the
format commit. It's integrated with IDEA[2] and GitHub Web UI[3][4] also.

Best,
tison.

[1]
https://stackoverflow.com/questions/5098256/how-can-i-view-prior-commits-with-git-blame
[2]
https://www.jetbrains.com/help/idea/viewing-changes-information.html#annotating-previous-versions
[3]
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view
[4] https://github.com/refined-github/refined-github/issues/2175


Yunze Xu <xy...@apache.org> 于2023年6月30日周五 11:38写道：

> One thing I’m concerned about is that the code reformatting could bring
> very
> wide changes and it’s very unfriendly to the `git blame` command.
>
> The check style plugin was not applied to all modules as well. There were
> many
> PRs to apply the check style plugin for different modules. Take my PR [1]
> as
> example, which applies the check style plugin for the pulsar-client
> module. There
> are over 2000 lines changes. It’s hard to review and it also brings a
> breaking
> change that was missed by the reviewers. You can see a public class
> `Murmur3_32Hash`
> was renamed with `Murmur3Hash32`.
>
> [1] https://github.com/apache/pulsar/pull/13940
>
> Thanks,
> Yunze
>
> On Fri, Jun 30, 2023 at 10:48 AM Joo Hyuk Kim <be...@gmail.com> wrote:
> >
> > > Spotless has an IDEA plugin, but it's for Gradle task only.
> >
> > I saw this one also 🥲.
> >
> > >  [1] can be used
> >
> > Nice, IMO having at least one will suffice 👍🏻.
> > Thanks,
> > joohyuk
> >
> > On Fri, Jun 30, 2023 at 11:43 AM tison <wa...@gmail.com> wrote:
> >
> > > > IDE
> > >
> > > If using the palantir style, [1] can be used. Spotless has an IDEA
> plugin
> > > but it's for Gradle task only.
> > >
> > > Best,
> > > tison.
> > >
> > > [1] https://plugins.jetbrains.com/plugin/13180-palantir-java-format
> > >
> > >
> > > Joo Hyuk Kim <be...@gmail.com> 于2023年6月30日周五 10:38写道：
> > >
> > > > Sounds great.
> > > >
> > > > Another thing is about IDE.
> > > > Pulsar site sort of directs contributors to use IntelliJ as shown in
> > > > https://pulsar.apache.org/contribute/setup-ide/.
> > > > It would be nice if there was IDE support (auto-format, or
> shortkey-base)
> > > > and we can add it to the setup-ide guide also.
> > > > Do you know any? Or I can do some research if you don't.
> > > >
> > > > Thanks,
> > > > joohyuk
> > > >
> > > > On Fri, Jun 30, 2023 at 10:58 AM tison <wa...@gmail.com> wrote:
> > > >
> > > > > 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
> > > > >
> > > >
> > >
>

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Yunze Xu <xy...@apache.org>]

One thing I’m concerned about is that the code reformatting could bring very
wide changes and it’s very unfriendly to the `git blame` command.

The check style plugin was not applied to all modules as well. There were many
PRs to apply the check style plugin for different modules. Take my PR [1] as
example, which applies the check style plugin for the pulsar-client
module. There
are over 2000 lines changes. It’s hard to review and it also brings a breaking
change that was missed by the reviewers. You can see a public class
`Murmur3_32Hash`
was renamed with `Murmur3Hash32`.

[1] https://github.com/apache/pulsar/pull/13940

Thanks,
Yunze

On Fri, Jun 30, 2023 at 10:48 AM Joo Hyuk Kim <be...@gmail.com> wrote:
>
> > Spotless has an IDEA plugin, but it's for Gradle task only.
>
> I saw this one also 🥲.
>
> >  [1] can be used
>
> Nice, IMO having at least one will suffice 👍🏻.
> Thanks,
> joohyuk
>
> On Fri, Jun 30, 2023 at 11:43 AM tison <wa...@gmail.com> wrote:
>
> > > IDE
> >
> > If using the palantir style, [1] can be used. Spotless has an IDEA plugin
> > but it's for Gradle task only.
> >
> > Best,
> > tison.
> >
> > [1] https://plugins.jetbrains.com/plugin/13180-palantir-java-format
> >
> >
> > Joo Hyuk Kim <be...@gmail.com> 于2023年6月30日周五 10:38写道：
> >
> > > Sounds great.
> > >
> > > Another thing is about IDE.
> > > Pulsar site sort of directs contributors to use IntelliJ as shown in
> > > https://pulsar.apache.org/contribute/setup-ide/.
> > > It would be nice if there was IDE support (auto-format, or shortkey-base)
> > > and we can add it to the setup-ide guide also.
> > > Do you know any? Or I can do some research if you don't.
> > >
> > > Thanks,
> > > joohyuk
> > >
> > > On Fri, Jun 30, 2023 at 10:58 AM tison <wa...@gmail.com> wrote:
> > >
> > > > 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
> > > >
> > >
> >

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Joo Hyuk Kim <be...@gmail.com>]

> Spotless has an IDEA plugin, but it's for Gradle task only.

I saw this one also 🥲.

>  [1] can be used

Nice, IMO having at least one will suffice 👍🏻.
Thanks,
joohyuk

On Fri, Jun 30, 2023 at 11:43 AM tison <wa...@gmail.com> wrote:

> > IDE
>
> If using the palantir style, [1] can be used. Spotless has an IDEA plugin
> but it's for Gradle task only.
>
> Best,
> tison.
>
> [1] https://plugins.jetbrains.com/plugin/13180-palantir-java-format
>
>
> Joo Hyuk Kim <be...@gmail.com> 于2023年6月30日周五 10:38写道：
>
> > Sounds great.
> >
> > Another thing is about IDE.
> > Pulsar site sort of directs contributors to use IntelliJ as shown in
> > https://pulsar.apache.org/contribute/setup-ide/.
> > It would be nice if there was IDE support (auto-format, or shortkey-base)
> > and we can add it to the setup-ide guide also.
> > Do you know any? Or I can do some research if you don't.
> >
> > Thanks,
> > joohyuk
> >
> > On Fri, Jun 30, 2023 at 10:58 AM tison <wa...@gmail.com> wrote:
> >
> > > 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
> > >
> >
>

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[tison <wa...@gmail.com>]

> IDE

If using the palantir style, [1] can be used. Spotless has an IDEA plugin
but it's for Gradle task only.

Best,
tison.

[1] https://plugins.jetbrains.com/plugin/13180-palantir-java-format


Joo Hyuk Kim <be...@gmail.com> 于2023年6月30日周五 10:38写道：

> Sounds great.
>
> Another thing is about IDE.
> Pulsar site sort of directs contributors to use IntelliJ as shown in
> https://pulsar.apache.org/contribute/setup-ide/.
> It would be nice if there was IDE support (auto-format, or shortkey-base)
> and we can add it to the setup-ide guide also.
> Do you know any? Or I can do some research if you don't.
>
> Thanks,
> joohyuk
>
> On Fri, Jun 30, 2023 at 10:58 AM tison <wa...@gmail.com> wrote:
>
> > 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
> >
>

Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools

[Joo Hyuk Kim <be...@gmail.com>]

Sounds great.

Another thing is about IDE.
Pulsar site sort of directs contributors to use IntelliJ as shown in
https://pulsar.apache.org/contribute/setup-ide/.
It would be nice if there was IDE support (auto-format, or shortkey-base)
and we can add it to the setup-ide guide also.
Do you know any? Or I can do some research if you don't.

Thanks,
joohyuk

On Fri, Jun 30, 2023 at 10:58 AM tison <wa...@gmail.com> wrote:

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