You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Zixuan Liu <no...@gmail.com> on 2024/03/18 09:27:57 UTC
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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
Posted by 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
> >
>