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