Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 于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
Top tison 于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
e: > > >>>>> > > >>>>> This idea looks good to me, but we need to format all codebase to > > >>>>> avoid conflicts during cherry picking. > > >>>>> > > >>>>> +1 > > >>>>> > > >>>>> Asaf Mesika 于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 > 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 于2023年8月16日 周三00:50写道： > > >>>>>>> > > >>>>>>>> Tison, > > >>>>>>>> > > >>>>>>>> Il Mar 15 Ago 2023, 16:56 tison 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 于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 co
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
ave 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 于2023年8月16日 周三00:50写道： > >>>>>>> > >>>>>>>> Tison, > >>>>>>>> > >>>>>>>> Il Mar 15 Ago 2023, 16:56 tison 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 于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 > >> wrote: > >>>>>>>>>> > >>>>>>>>>>> ... which seems a GitHub only extension - > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>> > >> https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > >>>>>>>>>>> > >>>>>>>>>>> Best, > >>>>>>>>>>> tison. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> tison 于2023年7月5日周三 19:38写道： > >>>>>>>>>>> > >>>>>>>>>>>> For the git blame issue, I found also this practice in > >>>>>>>> StreamPark[1]. > >>>>>>>>>>>> > >>>>>>>>>>>> cc @Yunze. > >>>>>>>>>>>> &
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
t;>>>>>>>> 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 于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 >> wrote: >>>>>>>>>> >>>>>>>>>>> ... which seems a GitHub only extension - >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>> >> https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ >>>>>>>>>>> >>>>>>>>>>> Best, >>>>>>>>>>> tison. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> tison 于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 于2023年6月30日周五 >>>>>>>> 13:03写道： >>>>>>>>>>>> >>>>>>>>>>>>> My mistake. It looks that for some reason Spotless supports >>>>>>>>>>> .editorconfig >>>>>>>>>>>>> only for ktlint. >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Kiryl >>>
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
; 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 于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 > wrote: > >>>>>>>> > >>>>>>>>> ... which seems a GitHub only extension - > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >> > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > >>>>>>>>> > >>>>>>>>> Best, > >>>>>>>>> tison. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> tison 于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 于2023年6月30日周五 > >>>>>> 13:03写道： > >>>>>>>>>> > >>>>>>>>>>> My mistake. It looks that for some reason Spotless supports > >>>>>>>>> .editorconfig > >>>>>>>>>>> only for ktlint. > >>>>>>>>>>> > >>>>>>>>>>> Best, > >>>>>>>>>>> Kiryl > >>>>>>>>>>> > >>>>>>>>>>> From: Kiryl Valkovich > >>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM > >>>>>>>>>>> To: dev@pulsar.apache.org > >>>>>>>>>>> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) > >>>>> and > >>>>>>>>>>> autotools > >>>>>>>>>>> Hi, > >>>>>>>>>>> > >>>>>>>>>>> tison, are you going to use .editorconfig for specifying indent > >>>>>>> rules? > >>>>>>>>>>> > >>>>>>>>>>> https://editorconfig.org/ > >>>>>>>>>>> > >>>>>>>>>>> It looks like Spotless supports it. >
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
t;>> >>>>>>>> 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 wrote: >>>>>>>> >>>>>>>>> ... which seems a GitHub only extension - >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >> https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> tison. >>>>>>>>> >>>>>>>>> >>>>>>>>> tison 于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 于2023年6月30日周五 >>>>>> 13:03写道： >>>>>>>>>> >>>>>>>>>>> My mistake. It looks that for some reason Spotless supports >>>>>>>>> .editorconfig >>>>>>>>>>> only for ktlint. >>>>>>>>>>> >>>>>>>>>>> Best, >>>>>>>>>>> Kiryl >>>>>>>>>>> >>>>>>>>>>> From: Kiryl Valkovich >>>>>>>>>>> Date: Friday, June 30, 2023 at 6:45 AM >>>>>>>>>>> To: dev@pulsar.apache.org >>>>>>>>>>> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) >>>>> and >>>>>>>>>>> autotools >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> tison, are you going to use .editorconfig for specifying indent >>>>>>> rules? >>>>>>>>>>> >>>>>>>>>>> https://editorconfig.org/ >>>>>>>>>>> >>>>>>>>>>> It looks like Spotless supports it. >>>>>>>>>>> https://github.com/diffplug/spotless/issues/734 >>>>>>>>>>> >>>>>>>>>>> Flink and many other ASF projects use it. >>>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >> https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig >>>>>>>>>>> >>>>>> https://github.com/search?q=org%3Aapache%20.editorconfig=code >>>>>>>>>>> >>>>>>>>>>> Unlike Spotless, the .editorconfig works out of the box in most >>>>> of >>>>>>> the >>>>>>>>>>> modern code editors. >>>>>>>>>>> >>>>>>>>>>> Best, >>>>>>>>>>> Kiryl >>>>>>>>>>> >>>>>>>>>>> From: tison >>>>>>>>>>> Date: Friday, June 30, 2023 at 3:58 AM >>>>>>>>>>> T
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 于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 wrote: > > > > This idea looks good to me, but we need to format all codebase to > > avoid conflicts during cherry picking. > > > > +1 > > > > Asaf Mesika 于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 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 于2023年8月16日 周三00:50写道： > >>> > >>>> Tison, > >>>> > >>>> Il Mar 15 Ago 2023, 16:56 tison 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 于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 wrote: > >>>>>> > >>>>>>> ... which seems a GitHub only extension - > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > >>>>>>> > >>>>>>> Best, > >>>>>>> tison. > >>>>>>> &g
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
-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 wrote: > > This idea looks good to me, but we need to format all codebase to > avoid conflicts during cherry picking. > > +1 > > Asaf Mesika 于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 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 于2023年8月16日 周三00:50写道： >>> >>>> Tison, >>>> >>>> Il Mar 15 Ago 2023, 16:56 tison 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 于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 wrote: >>>>>> >>>>>>> ... which seems a GitHub only extension - >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ >>>>>>> >>>>>>> Best, >>>>>>> tison. >>>>>>> >>>>>>> >>>>>>> tison 于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 于2023年6月30日周五 >>>> 13:03写道： >>>>>>>> >>&
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
This idea looks good to me, but we need to format all codebase to avoid conflicts during cherry picking. +1 Asaf Mesika 于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 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 于2023年8月16日 周三00:50写道： > > > > > Tison, > > > > > > Il Mar 15 Ago 2023, 16:56 tison 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 于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 wrote: > > > > > > > > > > > ... which seems a GitHub only extension - > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > > > > > > > > > > > > Best, > > > > > > tison. > > > > > > > > > > > > > > > > > > tison 于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 于2023年6月30日周五 > > > 13:03写道： > > > > > > > > > > > > > >> My mistake. It looks that for some reason Spotless supports > > > > > > .editorconfig > > > > > > >> only for ktlint. > > > > > > >> > > > > > > >> Best, > > > > > > >> Kiryl > > > > > > >> > > > > > > >> From: Kiryl Valkovich > > > > > > >> Date: Friday, June 30, 2023 at 6:45 AM > > > > >
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 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 于2023年8月16日 周三00:50写道： > > > Tison, > > > > Il Mar 15 Ago 2023, 16:56 tison 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 于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 wrote: > > > > > > > > > ... which seems a GitHub only extension - > > > > > > > > > > > > > > > > > > > > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > > > > > > > > > > Best, > > > > > tison. > > > > > > > > > > > > > > > tison 于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 于2023年6月30日周五 > > 13:03写道： > > > > > > > > > > > >> My mistake. It looks that for some reason Spotless supports > > > > > .editorconfig > > > > > >> only for ktlint. > > > > > >> > > > > > >> Best, > > > > > >> Kiryl > > > > > >> > > > > > >> From: Kiryl Valkovich > > > > > >> Date: Friday, June 30, 2023 at 6:45 AM > > > > > >> To: dev@pulsar.apache.org > > > > > >> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) > and > > > > > >> autotools > > > > > >> Hi, > > > > > >> > > > > > >> tison, are you going to use .editorconfig for specifying indent > > > rules? > > > > > >> > > > > > >> https://editorconfig.org/ > > > > > >> > > > > > >> It looks like Spotless supports it. > > > > > >> https://github.com/diffplug/spotless/issues/734 > > > > > >> > > > > > >
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 于2023年8月16日 周三00:50写道： > Tison, > > Il Mar 15 Ago 2023, 16:56 tison 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 于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 wrote: > > > > > > > ... which seems a GitHub only extension - > > > > > > > > > > > > > > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > > > > > > > > Best, > > > > tison. > > > > > > > > > > > > tison 于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 于2023年6月30日周五 > 13:03写道： > > > > > > > > > >> My mistake. It looks that for some reason Spotless supports > > > > .editorconfig > > > > >> only for ktlint. > > > > >> > > > > >> Best, > > > > >> Kiryl > > > > >> > > > > >> From: Kiryl Valkovich > > > > >> Date: Friday, June 30, 2023 at 6:45 AM > > > > >> To: dev@pulsar.apache.org > > > > >> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and > > > > >> autotools > > > > >> Hi, > > > > >> > > > > >> tison, are you going to use .editorconfig for specifying indent > > rules? > > > > >> > > > > >> https://editorconfig.org/ > > > > >> > > > > >> It looks like Spotless supports it. > > > > >> https://github.com/diffplug/spotless/issues/734 > > > > >> > > > > >> Flink and many other ASF projects use it. > > > > >> > > > > > > > > > > https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig > > > > >> > https://github.com/search?q=org%3Aapache%20.editorconfig=code > > > > >> > > > > >> Unlike Spotless, the .editorconfig works out of the box in most of > > the > > > > >> modern code editors. > > > > >> > > > > >> Best, > > > > >> Kiryl > > > > >> > > > > >> From: tison > > > > >> Date: Friday, June 30, 2023 at 3:58 AM > > > > >> To: Dev > > > > >> Subject: [DISCUSS] Consistent code style (esp. ws/indent) and > > > autotools > > > > >> Hi, > > > > >> > > > > >> I'd like to propose applying a consistent co
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
Tison, Il Mar 15 Ago 2023, 16:56 tison 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 于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 wrote: > > > > > ... which seems a GitHub only extension - > > > > > > > > > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > > > > > > Best, > > > tison. > > > > > > > > > tison 于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 于2023年6月30日周五 13:03写道： > > > > > > > >> My mistake. It looks that for some reason Spotless supports > > > .editorconfig > > > >> only for ktlint. > > > >> > > > >> Best, > > > >> Kiryl > > > >> > > > >> From: Kiryl Valkovich > > > >> Date: Friday, June 30, 2023 at 6:45 AM > > > >> To: dev@pulsar.apache.org > > > >> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and > > > >> autotools > > > >> Hi, > > > >> > > > >> tison, are you going to use .editorconfig for specifying indent > rules? > > > >> > > > >> https://editorconfig.org/ > > > >> > > > >> It looks like Spotless supports it. > > > >> https://github.com/diffplug/spotless/issues/734 > > > >> > > > >> Flink and many other ASF projects use it. > > > >> > > > > > > https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig > > > >> https://github.com/search?q=org%3Aapache%20.editorconfig=code > > > >> > > > >> Unlike Spotless, the .editorconfig works out of the box in most of > the > > > >> modern code editors. > > > >> > > > >> Best, > > > >> Kiryl > > > >> > > > >> From: tison > > > >> Date: Friday, June 30, 2023 at 3:58 AM > > > >> To: Dev > > > >> 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
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 于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 wrote: > > > ... which seems a GitHub only extension - > > > > > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > > > > Best, > > tison. > > > > > > tison 于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 于2023年6月30日周五 13:03写道： > > > > > >> My mistake. It looks that for some reason Spotless supports > > .editorconfig > > >> only for ktlint. > > >> > > >> Best, > > >> Kiryl > > >> > > >> From: Kiryl Valkovich > > >> Date: Friday, June 30, 2023 at 6:45 AM > > >> To: dev@pulsar.apache.org > > >> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and > > >> autotools > > >> Hi, > > >> > > >> tison, are you going to use .editorconfig for specifying indent rules? > > >> > > >> https://editorconfig.org/ > > >> > > >> It looks like Spotless supports it. > > >> https://github.com/diffplug/spotless/issues/734 > > >> > > >> Flink and many other ASF projects use it. > > >> > > > https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig > > >> https://github.com/search?q=org%3Aapache%20.editorconfig=code > > >> > > >> Unlike Spotless, the .editorconfig works out of the box in most of the > > >> modern code editors. > > >> > > >> Best, > > >> Kiryl > > >> > > >> From: tison > > >> Date: Friday, June 30, 2023 at 3:58 AM > > >> To: Dev > > >> 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? > > >> > > >&
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 wrote: > ... which seems a GitHub only extension - > > https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ > > Best, > tison. > > > tison 于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 于2023年6月30日周五 13:03写道： > > > >> My mistake. It looks that for some reason Spotless supports > .editorconfig > >> only for ktlint. > >> > >> Best, > >> Kiryl > >> > >> From: Kiryl Valkovich > >> Date: Friday, June 30, 2023 at 6:45 AM > >> To: dev@pulsar.apache.org > >> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and > >> autotools > >> Hi, > >> > >> tison, are you going to use .editorconfig for specifying indent rules? > >> > >> https://editorconfig.org/ > >> > >> It looks like Spotless supports it. > >> https://github.com/diffplug/spotless/issues/734 > >> > >> Flink and many other ASF projects use it. > >> > https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig > >> https://github.com/search?q=org%3Aapache%20.editorconfig=code > >> > >> Unlike Spotless, the .editorconfig works out of the box in most of the > >> modern code editors. > >> > >> Best, > >> Kiryl > >> > >> From: tison > >> Date: Friday, June 30, 2023 at 3:58 AM > >> To: Dev > >> 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 &g
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
... which seems a GitHub only extension - https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ Best, tison. tison 于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 于2023年6月30日周五 13:03写道： > >> My mistake. It looks that for some reason Spotless supports .editorconfig >> only for ktlint. >> >> Best, >> Kiryl >> >> From: Kiryl Valkovich >> Date: Friday, June 30, 2023 at 6:45 AM >> To: dev@pulsar.apache.org >> Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and >> autotools >> Hi, >> >> tison, are you going to use .editorconfig for specifying indent rules? >> >> https://editorconfig.org/ >> >> It looks like Spotless supports it. >> https://github.com/diffplug/spotless/issues/734 >> >> Flink and many other ASF projects use it. >> https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig >> https://github.com/search?q=org%3Aapache%20.editorconfig=code >> >> Unlike Spotless, the .editorconfig works out of the box in most of the >> modern code editors. >> >> Best, >> Kiryl >> >> From: tison >> Date: Friday, June 30, 2023 at 3:58 AM >> To: Dev >> 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
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 于2023年6月30日周五 13:03写道： > My mistake. It looks that for some reason Spotless supports .editorconfig > only for ktlint. > > Best, > Kiryl > > From: Kiryl Valkovich > Date: Friday, June 30, 2023 at 6:45 AM > To: dev@pulsar.apache.org > Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools > Hi, > > tison, are you going to use .editorconfig for specifying indent rules? > > https://editorconfig.org/ > > It looks like Spotless supports it. > https://github.com/diffplug/spotless/issues/734 > > Flink and many other ASF projects use it. > https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig > https://github.com/search?q=org%3Aapache%20.editorconfig=code > > Unlike Spotless, the .editorconfig works out of the box in most of the > modern code editors. > > Best, > Kiryl > > From: tison > Date: Friday, June 30, 2023 at 3:58 AM > To: Dev > 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
My mistake. It looks that for some reason Spotless supports .editorconfig only for ktlint. Best, Kiryl From: Kiryl Valkovich Date: Friday, June 30, 2023 at 6:45 AM To: dev@pulsar.apache.org Subject: Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools Hi, tison, are you going to use .editorconfig for specifying indent rules? https://editorconfig.org/ It looks like Spotless supports it. https://github.com/diffplug/spotless/issues/734 Flink and many other ASF projects use it. https://github.com/apache/flink/blob/21eba4ca4cb235a2189c94cdbf3abcec5cde1e6e/.editorconfig https://github.com/search?q=org%3Aapache%20.editorconfig=code Unlike Spotless, the .editorconfig works out of the box in most of the modern code editors. Best, Kiryl From: tison Date: Friday, June 30, 2023 at 3:58 AM To: Dev 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
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=code Unlike Spotless, the .editorconfig works out of the box in most of the modern code editors. Best, Kiryl From: tison Date: Friday, June 30, 2023 at 3:58 AM To: Dev 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
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 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 于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 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 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 于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 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
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
`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 于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 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 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 于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 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
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
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 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 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 于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 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]
Re: [DISCUSS] Consistent code style (esp. ws/indent) and autotools
> 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 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 于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 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
> 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 于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 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
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 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 >
[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
