You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@storm.apache.org by "sercuzz8 (via GitHub)" <gi...@apache.org> on 2026/08/28 16:04:02 UTC
[PR] Add checkstyle auto-fix tooling (storm)
sercuzz8 opened a new pull request, #9017:
URL: https://github.com/apache/storm/pull/9017
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-6236936575
> @sercuzz8 @GGraziadei is there any outstanding work on this one?
I think I have addressed all the points. Agree, @GGraziadei?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-6004193200
Hello, thank you for the heads up, I must have messed up during the squash.
I have reinstated the pom and checked both with
`mvn validate`
`mvn org.openrewrite.maven:rewrite-maven-plugin:run@checkstyle-autofix-openrewrite org.codehaus.gmavenplus:gmavenplus-plugin:execute@checkstyle-autofix org.openrewrite.maven:rewrite-maven-plugin:run@checkstyle-reindent-openrewrite -pl '!storm-shaded-deps' -Dcheckstyle.skip=true `
and they work
In the process, I noticed that there are some deprecated methods in the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5735959859
Hi @sercuzz8 any updates on this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4262518712
##########
storm-checkstyle/src/main/resources/storm/normalize-license-headers.groovy:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * Rewrites the leading license comment of the files of a module (Java, Groovy, XML, Markdown,
+ * YAML, properties, Python and shell) to the exact template of
+ * https://www.apache.org/legal/src-headers.html#headers.
+ *
+ * A file is left alone when its leading comment is not an ASF license header, or when it carries
+ * a copyright notice of anybody but the ASF (third-party works keep their own notice).
+ *
+ * Run on demand via gmavenplus-plugin's `execute` goal (execution id `normalize-license-headers`,
+ * bound to no phase); see storm-checkstyle/README.md. With -Dlicense.check=true nothing is
+ * written and the build fails if a file would change.
+ */
+
+final List<String> TEXT = '''\
Review Comment:
Hello @GGraziadei, I have rechecked and indeed both license headers were accepted but the "to You" version was still touched due to some other perks (e.g. the double spaces after the period in ". See").
I have made addressed it with the first new commit, furthermore, I have made the groovy autofix files themselves untouchable in the second new one
##########
storm-checkstyle/src/main/resources/storm/normalize-license-headers.groovy:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * Rewrites the leading license comment of the files of a module (Java, Groovy, XML, Markdown,
+ * YAML, properties, Python and shell) to the exact template of
+ * https://www.apache.org/legal/src-headers.html#headers.
+ *
+ * A file is left alone when its leading comment is not an ASF license header, or when it carries
+ * a copyright notice of anybody but the ASF (third-party works keep their own notice).
+ *
+ * Run on demand via gmavenplus-plugin's `execute` goal (execution id `normalize-license-headers`,
+ * bound to no phase); see storm-checkstyle/README.md. With -Dlicense.check=true nothing is
+ * written and the build fails if a file would change.
+ */
+
+final List<String> TEXT = '''\
Review Comment:
Hello @GGraziadei, I have rechecked and indeed both license headers were accepted but the "to You" version was still touched due to some other perks (e.g. the double spaces after the period in ". See").
I have addressed it with the first new commit, furthermore, I have made the groovy autofix files themselves untouchable in the second new one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-6239303481
Hi @reiabreu, @sercuzz8
Thanks!
All the points addressed, just the time to complete my review with a manual check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4304397466
##########
storm-checkstyle/README.md:
##########
@@ -0,0 +1,144 @@
+# Checkstyle auto-fix
+
+`storm_checkstyle.xml` is the Checkstyle ruleset enforced on every `mvn` build.
+Many rules are `severity=warning` as they do not fail the build, but they should still be
+slimmed to the minimal amount
+
+The auto-fix is **not** part of the normal build. It is run on demand in two stages,
+in every module that declares the plugins:
+
+1. **`rewrite-maven-plugin` (OpenRewrite)** - reduced amount of fixes done on a parsed Java AST.
+ The recipe list is `rewrite.yml` at the repo root (`org.apache.storm.checkstyle.AutoFix`).
+2. **`src/main/resources/storm/checkstyle-autofix.groovy`** (via `gmavenplus-plugin`) -
+ runs Checkstyle itself, in-process, with the same version and ruleset the build
+ uses, reads violations (file, line, column, message) and hands each one to a small fixer written for the one
+ Checkstyle module that reported it. The cycle until no more rules can be fixed.
+
+The values a fixer needs (`Indentation` `basicOffset`, `LineLength` `max`,
+`CustomImportOrder` rules) are read from the ruleset XML at run time avoiding hardcoding. Files that the
+module excludes from Checkstyle (`<excludes>` of `maven-checkstyle-plugin`, e.g. thrift
+generated code) are neither audited nor rewritten.
+
+The license comment at the top of a file is never touched, whatever Checkstyle reports in it
+(`LineLength` on a wide-wrapped header, `JavadocParagraph` on one opened with `/**`): both
+wrappings of the ASF header are accepted, see [License headers](#license-headers).
Review Comment:
Thank you, I had not noticed it. I have integrated the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4057121054
##########
.github/workflows/checkstyle-autofix.yaml:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Runs the checkstyle auto-fix (storm-checkstyle/README.md) on one module and pushes the result to
+# the branch checkstyle-fix/<module>, from which a pull request against master can be opened.
+name: Checkstyle auto-fix
+
+on:
+ workflow_dispatch:
+ inputs:
+ module:
+ description: 'Maven module to fix, as its path in the repository (e.g. storm-client or external/storm-hdfs)'
+ required: true
+ type: string
+
+permissions:
+ contents: write
+
+jobs:
+ autofix:
+ runs-on: ubuntu-latest
+ timeout-minutes: 120
+ env:
+ MODULE: ${{ inputs.module }}
+ steps:
+ - name: Validate the module input
+ run: |
+ if ! [[ "$MODULE" =~ ^[A-Za-z0-9._-]+(/[A-Za-z0-9._-]+)*$ ]] || [[ "$MODULE" == *..* ]]; then
+ echo "::error::'$MODULE' is not a valid module path"
+ exit 1
+ fi
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ - name: Check that the module exists
+ run: test -f "$MODULE/pom.xml" || { echo "::error::$MODULE/pom.xml not found"; exit 1; }
+ - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
+ with:
+ path: ~/.m2/repository
+ key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
+ restore-keys: |
+ ${{ runner.os }}-maven-
+ - name: Set up JDK 25
+ uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1
+ with:
+ distribution: temurin
+ java-version: 25
+ - name: Build the module and the modules it depends on (the Groovy stage resolves its classpath)
+ run: mvn --batch-mode install -DskipTests -Dcheckstyle.skip=true -pl "$MODULE" -am
Review Comment:
I advice to execute a checkstyle before and after including the results inside the job artifact of GitHub action.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5548861353
@GGraziadei are you happy that this fulfils your request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-6235642851
@sercuzz8 @GGraziadei is there any outstanding work on this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5749342800
Hi @sercuzz8 very good progress on this side. Thanks!
The next steps:
### Migrate from Groovy to OpenRewrite
Please proceed according to this table if a OpenRewrite rule is available.
| Checkstyle module | Groovy fixer today | OpenRewrite recipe | Coverage | Notes |
|---|---|---|---|---|
| `NeedBraces` | yes (fallback) | `org.openrewrite.staticanalysis.NeedBraces` | Full | already in `rewrite.yml` |
| `MultipleVariableDeclarations` | yes (fallback) | `org.openrewrite.staticanalysis.MultipleVariableDeclarations` | Full | already in `rewrite.yml` |
| `ModifierOrder` | yes (fallback) | `org.openrewrite.staticanalysis.ModifierOrder` | Full | already in `rewrite.yml` |
| `UpperEll` | yes (fallback) | `org.openrewrite.staticanalysis.UpperCaseLiteralSuffixes` | Full | already in `rewrite.yml` |
| `ArrayTypeStyle` | yes (fallback) | `org.openrewrite.staticanalysis.UseJavaStyleArrayDeclarations` | Full | already in `rewrite.yml` |
| `WhitespaceAround`, `WhitespaceAfter`, `NoWhitespaceBefore` | yes | `org.openrewrite.java.format.Spaces` | Style | `SpacesStyle` must mirror the ruleset; whole-file reformat |
| `ParenPad`, `MethodParamPad`, `GenericWhitespace` | yes | `org.openrewrite.java.format.Spaces` | Style | same `SpacesStyle` |
| `NoWhitespaceBeforeCaseDefaultColon` | yes | `org.openrewrite.java.format.Spaces` | Partial (verify) | not sure the style has a knob for `case X :` |
| `RegexpSinglelineJava` (empty-block `{}` spacing) | yes | `org.openrewrite.java.format.Spaces` | Partial (verify) | regex-based rule, no 1:1 recipe |
| `FileTabCharacter` | yes | `org.openrewrite.java.format.NormalizeTabsOrSpaces` | Full | |
| `Indentation` | yes | `org.openrewrite.java.format.TabsAndIndents` | Style | `TabsAndIndentsStyle` with `basicOffset` and continuation indent |
| `CommentsIndentation` | yes | `org.openrewrite.java.format.TabsAndIndents` | Partial | comments follow the enclosing indent, not every case Checkstyle checks |
| `EmptyLineSeparator` | yes | `org.openrewrite.java.format.BlankLines` | Style | `BlankLinesStyle` |
| `LeftCurly`, `RightCurly` | yes | `org.openrewrite.java.format.WrappingAndBraces` | Style | the `RightCurly alone` regression the PR saw comes from the Checkstyle-derived style, not the recipe; a custom `WrappingAndBracesStyle` avoids it |
| `NoLineWrap` | yes | `org.openrewrite.java.format.WrappingAndBraces` | Partial | package/import wraps only |
| `AnnotationLocation` | yes | `org.openrewrite.java.format.WrappingAndBraces` | Partial | annotations on their own line for types/methods; field and parameter cases not covered |
| `OneStatementPerLine` | yes | — | None | small custom visitor |
| `OperatorWrap` | yes | `org.openrewrite.staticanalysis.OperatorWrap` | Full | has an `nl`/`eol` option; use `nl` to match the ruleset |
| `SeparatorWrap` | yes | — | None | comma/dot wrap position; custom visitor |
| `LineLength` | yes (comments, arguments, `&&`/`\|\|`/`+`, call chains, `=`, string split) | — | None | OpenRewrite has no column-based wrapping; this is the largest Groovy fixer and stays |
| `CustomImportOrder` | yes | `org.openrewrite.java.OrderImports` | Style | `ImportLayoutStyle` with the ruleset's group order; set `classCountToUseStarImport` and `nameCountToUseStarImport` very high, which removes the "collapses to star imports" problem the PR hit |
| `AvoidStarImport` | yes (expands via module classpath) | `org.openrewrite.java.RemoveUnusedImports` | Partial (verify) | unfolds star imports only with type attribution, i.e. the module's dependencies resolvable, the same constraint the Groovy has |
| `IllegalTokenText`, `AvoidEscapedUnicodeCharacters` | yes | — | None | trivial custom recipes |
| `TodoComment`, single-line comment space (`MatchXpath`) | yes | — | None | trivial custom recipes |
| `OverloadMethodsDeclarationOrder` | yes | — | None | custom visitor reordering class members |
| `ConstructorsDeclarationGrouping` | yes | — | None | same visitor |
| `MissingSwitchDefault` | yes (adds `default: break;`) | `org.openrewrite.staticanalysis.DefaultComesLast` | Partial | only reorders an existing `default`; adding one needs a custom recipe |
| `JavadocLeadingAsteriskAlign`, `JavadocMissingLeadingAsterisk` | yes | — | None | the Javadoc LST exists, but no formatting recipes ship |
| `JavadocContentLocation`, `JavadocParagraph`, `JavadocTagContinuationIndentation` | yes | — | None | custom Javadoc visitors |
| `RequireEmptyLineBeforeBlockTagGroup`, `AtclauseOrder`, `InvalidJavadocPosition` | yes | — | None | custom Javadoc visitors |
| `SummaryJavadoc` (missing period, lowercase first word) | yes | — | None | custom Javadoc visitor |
### Fix the license note
The correct license note is this one https://www.apache.org/legal/src-headers.html#headers
Please evict to add additional `<p>` or other characters and uniform between the whole codebase.
### Trasform this PR in a GitHub action
The amount of code touched by this PR is too large to review. Could you drop the source fixes from this PR and keep only the tooling, and then add a GitHub Actions workflow (`workflow_dispatch`) that:
1. takes the module to fix as an input;
2. runs the auto-fix on that module;
3. pushes the result to a `checkstyle-fix/<module>` branch, so we can open one PR per module against `master`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4304283792
##########
storm-checkstyle/README.md:
##########
@@ -0,0 +1,144 @@
+# Checkstyle auto-fix
+
+`storm_checkstyle.xml` is the Checkstyle ruleset enforced on every `mvn` build.
+Many rules are `severity=warning` as they do not fail the build, but they should still be
+slimmed to the minimal amount
+
+The auto-fix is **not** part of the normal build. It is run on demand in two stages,
+in every module that declares the plugins:
+
+1. **`rewrite-maven-plugin` (OpenRewrite)** - reduced amount of fixes done on a parsed Java AST.
+ The recipe list is `rewrite.yml` at the repo root (`org.apache.storm.checkstyle.AutoFix`).
+2. **`src/main/resources/storm/checkstyle-autofix.groovy`** (via `gmavenplus-plugin`) -
+ runs Checkstyle itself, in-process, with the same version and ruleset the build
+ uses, reads violations (file, line, column, message) and hands each one to a small fixer written for the one
+ Checkstyle module that reported it. The cycle until no more rules can be fixed.
+
+The values a fixer needs (`Indentation` `basicOffset`, `LineLength` `max`,
+`CustomImportOrder` rules) are read from the ruleset XML at run time avoiding hardcoding. Files that the
+module excludes from Checkstyle (`<excludes>` of `maven-checkstyle-plugin`, e.g. thrift
+generated code) are neither audited nor rewritten.
+
+The license comment at the top of a file is never touched, whatever Checkstyle reports in it
+(`LineLength` on a wide-wrapped header, `JavadocParagraph` on one opened with `/**`): both
+wrappings of the ASF header are accepted, see [License headers](#license-headers).
Review Comment:
Thanks, verified locally on `storm-submit-tools` at 25327fbc: the headers are left alone now (36 -> 20 warnings, 0 header lines touched, module compiles and its tests pass).
Since both header wrappings are accepted, Checkstyle should stop reporting them too, otherwise every `/**` header keeps producing two `JavadocParagraph` warnings (lines 9 and 11) in the before/after reports of the workflow: 12 of the 20 remaining warnings on this module are exactly that, and it is the same on the other ~1450 files with that header.
A `SuppressionSingleFilter` in `storm_checkstyle.xml`, as a child of `Checker` right after the existing `SuppressionFilter`, does it:
```xml
<!-- https://checkstyle.org/filters/suppressionsinglefilter.html
Both wrappings of the ASF license header are accepted; the Javadoc one ('/**') trips
JavadocParagraph on its blank lines, so the check is muted on the header lines only. -->
<module name="SuppressionSingleFilter">
<property name="checks" value="JavadocParagraph"/>
<property name="lines" value="1-20"/>
</module>
```
With it the module goes to 8 warnings (0 `JavadocParagraph`), and a real `JavadocParagraph` violation added on a method below the imports is still reported, so only the header is muted. Note for whoever tries it: the plugin reads the ruleset from the installed `storm-checkstyle` jar, so `mvn install -pl storm-checkstyle` is needed before the change shows up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei merged PR #9017:
URL: https://github.com/apache/storm/pull/9017
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4057124919
##########
.github/workflows/checkstyle-autofix.yaml:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Runs the checkstyle auto-fix (storm-checkstyle/README.md) on one module and pushes the result to
+# the branch checkstyle-fix/<module>, from which a pull request against master can be opened.
+name: Checkstyle auto-fix
+
+on:
+ workflow_dispatch:
+ inputs:
+ module:
+ description: 'Maven module to fix, as its path in the repository (e.g. storm-client or external/storm-hdfs)'
+ required: true
+ type: string
+
+permissions:
+ contents: write
+
+jobs:
+ autofix:
+ runs-on: ubuntu-latest
+ timeout-minutes: 120
+ env:
+ MODULE: ${{ inputs.module }}
+ steps:
+ - name: Validate the module input
+ run: |
+ if ! [[ "$MODULE" =~ ^[A-Za-z0-9._-]+(/[A-Za-z0-9._-]+)*$ ]] || [[ "$MODULE" == *..* ]]; then
+ echo "::error::'$MODULE' is not a valid module path"
+ exit 1
+ fi
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ - name: Check that the module exists
+ run: test -f "$MODULE/pom.xml" || { echo "::error::$MODULE/pom.xml not found"; exit 1; }
+ - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
+ with:
+ path: ~/.m2/repository
+ key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
+ restore-keys: |
+ ${{ runner.os }}-maven-
+ - name: Set up JDK 25
+ uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1
+ with:
+ distribution: temurin
+ java-version: 25
+ - name: Build the module and the modules it depends on (the Groovy stage resolves its classpath)
+ run: mvn --batch-mode install -DskipTests -Dcheckstyle.skip=true -pl "$MODULE" -am
+ - name: Auto-fix the module (OpenRewrite, Groovy, OpenRewrite re-indent)
+ run: |
+ mvn --batch-mode -pl "$MODULE" -Dcheckstyle.skip=true \
+ org.openrewrite.maven:rewrite-maven-plugin:run@checkstyle-autofix-openrewrite \
+ org.codehaus.gmavenplus:gmavenplus-plugin:execute@checkstyle-autofix \
+ org.openrewrite.maven:rewrite-maven-plugin:run@checkstyle-reindent-openrewrite
+ - name: Normalize the license headers
+ run: mvn --batch-mode -pl "$MODULE" -Dcheckstyle.skip=true org.codehaus.gmavenplus:gmavenplus-plugin:execute@normalize-license-headers
+ - name: Push the result to checkstyle-fix/<module>
+ run: |
+ BRANCH="checkstyle-fix/$MODULE"
Review Comment:
This action executes checkout form master. Consider to create a branch for each execution of the action. Once merged the PR it will be deleted.
##########
storm-checkstyle/src/main/resources/storm/normalize-license-headers.groovy:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * Rewrites the leading license comment of the files of a module (Java, Groovy, XML, Markdown,
+ * YAML, properties, Python and shell) to the exact template of
+ * https://www.apache.org/legal/src-headers.html#headers.
+ *
+ * A file is left alone when its leading comment is not an ASF license header, or when it carries
+ * a copyright notice of anybody but the ASF (third-party works keep their own notice).
+ *
+ * Run on demand via gmavenplus-plugin's `execute` goal (execution id `normalize-license-headers`,
+ * bound to no phase); see storm-checkstyle/README.md. With -Dlicense.check=true nothing is
+ * written and the build fails if a file would change.
+ */
+
+final List<String> TEXT = '''\
Review Comment:
The code base currently uses two versions of the ASF header, both accepted by RAT.
The text is identical; they only differ in where the lines wrap.
Could the normalization accept both forms and leave them alone? Rewriting one into
the other touches ~1300 Java files and 40 poms and pollutes `git blame` on every file
for no legal or functional gain.
##########
.github/workflows/checkstyle-autofix.yaml:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Runs the checkstyle auto-fix (storm-checkstyle/README.md) on one module and pushes the result to
+# the branch checkstyle-fix/<module>, from which a pull request against master can be opened.
+name: Checkstyle auto-fix
+
+on:
+ workflow_dispatch:
+ inputs:
+ module:
+ description: 'Maven module to fix, as its path in the repository (e.g. storm-client or external/storm-hdfs)'
+ required: true
+ type: string
+
+permissions:
+ contents: write
+
+jobs:
+ autofix:
+ runs-on: ubuntu-latest
+ timeout-minutes: 120
+ env:
+ MODULE: ${{ inputs.module }}
+ steps:
+ - name: Validate the module input
+ run: |
+ if ! [[ "$MODULE" =~ ^[A-Za-z0-9._-]+(/[A-Za-z0-9._-]+)*$ ]] || [[ "$MODULE" == *..* ]]; then
+ echo "::error::'$MODULE' is not a valid module path"
+ exit 1
+ fi
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ - name: Check that the module exists
+ run: test -f "$MODULE/pom.xml" || { echo "::error::$MODULE/pom.xml not found"; exit 1; }
+ - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
+ with:
+ path: ~/.m2/repository
+ key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
+ restore-keys: |
+ ${{ runner.os }}-maven-
+ - name: Set up JDK 25
+ uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1
+ with:
+ distribution: temurin
+ java-version: 25
+ - name: Build the module and the modules it depends on (the Groovy stage resolves its classpath)
+ run: mvn --batch-mode install -DskipTests -Dcheckstyle.skip=true -pl "$MODULE" -am
+ - name: Auto-fix the module (OpenRewrite, Groovy, OpenRewrite re-indent)
+ run: |
+ mvn --batch-mode -pl "$MODULE" -Dcheckstyle.skip=true \
+ org.openrewrite.maven:rewrite-maven-plugin:run@checkstyle-autofix-openrewrite \
+ org.codehaus.gmavenplus:gmavenplus-plugin:execute@checkstyle-autofix \
+ org.openrewrite.maven:rewrite-maven-plugin:run@checkstyle-reindent-openrewrite
+ - name: Normalize the license headers
+ run: mvn --batch-mode -pl "$MODULE" -Dcheckstyle.skip=true org.codehaus.gmavenplus:gmavenplus-plugin:execute@normalize-license-headers
+ - name: Push the result to checkstyle-fix/<module>
+ run: |
+ BRANCH="checkstyle-fix/$MODULE"
+ # only the module itself (build output is git-ignored), never the tooling files
+ git add -A -- "$MODULE"
+ if git diff --cached --quiet; then
+ echo "Nothing to fix in $MODULE"
+ exit 0
+ fi
+ git config user.name "github-actions[bot]"
+ git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
+ git checkout -b "$BRANCH"
+ git commit -m "Checkstyle auto-fix of $MODULE"
+ git push --force origin "$BRANCH"
+ echo "Pushed $BRANCH; open a pull request against master from it." >> "$GITHUB_STEP_SUMMARY"
Review Comment:
Please execute again the checkstyle reporting the missing warnings, if any.
The GitHub action can create a PR directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5550607034
Hi @sercuzz8,
Good attempt! However, this plugin doesn’t fix all the warnings.
Have you tried to add more rules for org.codehaus.gmavenplus? Have you tried a different tool (with a parser support)?
Otherwise, I propose we keep the code changes and formatting, but exclude this plugin from `mvn validate`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4304283107
##########
storm-checkstyle/README.md:
##########
@@ -0,0 +1,144 @@
+# Checkstyle auto-fix
+
+`storm_checkstyle.xml` is the Checkstyle ruleset enforced on every `mvn` build.
+Many rules are `severity=warning` as they do not fail the build, but they should still be
+slimmed to the minimal amount
+
+The auto-fix is **not** part of the normal build. It is run on demand in two stages,
+in every module that declares the plugins:
+
+1. **`rewrite-maven-plugin` (OpenRewrite)** - reduced amount of fixes done on a parsed Java AST.
+ The recipe list is `rewrite.yml` at the repo root (`org.apache.storm.checkstyle.AutoFix`).
+2. **`src/main/resources/storm/checkstyle-autofix.groovy`** (via `gmavenplus-plugin`) -
+ runs Checkstyle itself, in-process, with the same version and ruleset the build
+ uses, reads violations (file, line, column, message) and hands each one to a small fixer written for the one
+ Checkstyle module that reported it. The cycle until no more rules can be fixed.
+
+The values a fixer needs (`Indentation` `basicOffset`, `LineLength` `max`,
+`CustomImportOrder` rules) are read from the ruleset XML at run time avoiding hardcoding. Files that the
+module excludes from Checkstyle (`<excludes>` of `maven-checkstyle-plugin`, e.g. thrift
+generated code) are neither audited nor rewritten.
+
+The license comment at the top of a file is never touched, whatever Checkstyle reports in it
+(`LineLength` on a wide-wrapped header, `JavadocParagraph` on one opened with `/**`): both
+wrappings of the ASF header are accepted, see [License headers](#license-headers).
Review Comment:
Thanks, verified locally on `storm-submit-tools` at 25327fbc: the headers are left alone now (36 -> 20 warnings, 0 header lines touched, module compiles and its tests pass).
Since both header wrappings are accepted, Checkstyle should stop reporting them too, otherwise every `/**` header keeps producing two `JavadocParagraph` warnings (lines 9 and 11) in the before/after reports of the workflow: 12 of the 20 remaining warnings on this module are exactly that, and it is the same on the other ~1450 files with that header.
A `SuppressionSingleFilter` in `storm_checkstyle.xml`, as a child of `Checker` right after the existing `SuppressionFilter`, does it:
```xml
<!-- https://checkstyle.org/filters/suppressionsinglefilter.html
Both wrappings of the ASF license header are accepted; the Javadoc one ('/**') trips
JavadocParagraph on its blank lines, so the check is muted on the header lines only. -->
<module name="SuppressionSingleFilter">
<property name="checks" value="JavadocParagraph"/>
<property name="lines" value="1-20"/>
</module>
```
With it the module goes to 8 warnings (0 `JavadocParagraph`), and a real `JavadocParagraph` violation added on a method below the imports is still reported, so only the header is muted. Note for whoever tries it: the plugin reads the ruleset from the installed `storm-checkstyle` jar, so `mvn install -pl storm-checkstyle` is needed before the change shows up.
##########
storm-checkstyle/README.md:
##########
@@ -0,0 +1,144 @@
+# Checkstyle auto-fix
+
+`storm_checkstyle.xml` is the Checkstyle ruleset enforced on every `mvn` build.
+Many rules are `severity=warning` as they do not fail the build, but they should still be
+slimmed to the minimal amount
+
+The auto-fix is **not** part of the normal build. It is run on demand in two stages,
+in every module that declares the plugins:
+
+1. **`rewrite-maven-plugin` (OpenRewrite)** - reduced amount of fixes done on a parsed Java AST.
+ The recipe list is `rewrite.yml` at the repo root (`org.apache.storm.checkstyle.AutoFix`).
+2. **`src/main/resources/storm/checkstyle-autofix.groovy`** (via `gmavenplus-plugin`) -
+ runs Checkstyle itself, in-process, with the same version and ruleset the build
+ uses, reads violations (file, line, column, message) and hands each one to a small fixer written for the one
+ Checkstyle module that reported it. The cycle until no more rules can be fixed.
+
+The values a fixer needs (`Indentation` `basicOffset`, `LineLength` `max`,
+`CustomImportOrder` rules) are read from the ruleset XML at run time avoiding hardcoding. Files that the
+module excludes from Checkstyle (`<excludes>` of `maven-checkstyle-plugin`, e.g. thrift
+generated code) are neither audited nor rewritten.
+
+The license comment at the top of a file is never touched, whatever Checkstyle reports in it
+(`LineLength` on a wide-wrapped header, `JavadocParagraph` on one opened with `/**`): both
+wrappings of the ASF header are accepted, see [License headers](#license-headers).
Review Comment:
Thanks, verified locally on `storm-submit-tools` at 25327fbc: the headers are left alone now (36 -> 20 warnings, 0 header lines touched, module compiles and its tests pass).
Since both header wrappings are accepted, Checkstyle should stop reporting them too, otherwise every `/**` header keeps producing two `JavadocParagraph` warnings (lines 9 and 11) in the before/after reports of the workflow: 12 of the 20 remaining warnings on this module are exactly that, and it is the same on the other ~1450 files with that header.
A `SuppressionSingleFilter` in `storm_checkstyle.xml`, as a child of `Checker` right after the existing `SuppressionFilter`, does it:
```xml
<!-- https://checkstyle.org/filters/suppressionsinglefilter.html
Both wrappings of the ASF license header are accepted; the Javadoc one ('/**') trips
JavadocParagraph on its blank lines, so the check is muted on the header lines only. -->
<module name="SuppressionSingleFilter">
<property name="checks" value="JavadocParagraph"/>
<property name="lines" value="1-20"/>
</module>
```
With it the module goes to 8 warnings (0 `JavadocParagraph`), and a real `JavadocParagraph` violation added on a method below the imports is still reported, so only the header is muted. Note for whoever tries it: the plugin reads the ruleset from the installed `storm-checkstyle` jar, so `mvn install -pl storm-checkstyle` is needed before the change shows up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5736467574
Hello @GGraziadei,
I am sorry for the delay. I have been trying to either expand this solution or use other tools (Openrewrite, Spotless) to eradicate warning.
Unfortunately I have not managed to overcome those regarding line length as breaking them sometimes breaks compilation in the process.
Given these conditions, I suppose that the best idea would be to remove this plugin from the validate, as you said.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "reiabreu (via GitHub)" <gi...@apache.org>.
reiabreu commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5993356502
*This comment was generated with the help of an LLM (Claude) and reviewed by me before posting.*
@sercuzz8 the main `pom.xml` is empty in this PR, so the project can't be built. It was emptied by the first commit (`8ae1027aa`). It looks like an accident. Could you restore it and re-add only the changes you meant to make?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-6250422344
Thanks for the contribution @sercuzz8
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4257343074
##########
storm-checkstyle/src/main/resources/storm/normalize-license-headers.groovy:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * Rewrites the leading license comment of the files of a module (Java, Groovy, XML, Markdown,
+ * YAML, properties, Python and shell) to the exact template of
+ * https://www.apache.org/legal/src-headers.html#headers.
+ *
+ * A file is left alone when its leading comment is not an ASF license header, or when it carries
+ * a copyright notice of anybody but the ASF (third-party works keep their own notice).
+ *
+ * Run on demand via gmavenplus-plugin's `execute` goal (execution id `normalize-license-headers`,
+ * bound to no phase); see storm-checkstyle/README.md. With -Dlicense.check=true nothing is
+ * written and the build fails if a file would change.
+ */
+
+final List<String> TEXT = '''\
Review Comment:
Hi @sercuzz8 can you please check if after [9ed670f](https://github.com/apache/storm/pull/9017/commits/9ed670f0b8d2dd8be496a0ad7f596cd0e3de4ece) both the license headers are now accepted by the style fixer?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-5742605685
Update: I took the need for a rebase as an occasion to improve the implementation.
It now uses Openrewrite as much as possible and complements with the original groovy script.
Some violations can't be solved mechanically (missing Javadoc, long lines being unbreakable URLs) so I removed the script from `mvn validate`.
The second commit contains the result of the run of the command
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "sercuzz8 (via GitHub)" <gi...@apache.org>.
sercuzz8 commented on PR #9017:
URL: https://github.com/apache/storm/pull/9017#issuecomment-6251033035
Thank you for the pieces of advice and patience
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle auto-fix tooling (storm)
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
GGraziadei commented on code in PR #9017:
URL: https://github.com/apache/storm/pull/9017#discussion_r4304283107
##########
storm-checkstyle/README.md:
##########
@@ -0,0 +1,144 @@
+# Checkstyle auto-fix
+
+`storm_checkstyle.xml` is the Checkstyle ruleset enforced on every `mvn` build.
+Many rules are `severity=warning` as they do not fail the build, but they should still be
+slimmed to the minimal amount
+
+The auto-fix is **not** part of the normal build. It is run on demand in two stages,
+in every module that declares the plugins:
+
+1. **`rewrite-maven-plugin` (OpenRewrite)** - reduced amount of fixes done on a parsed Java AST.
+ The recipe list is `rewrite.yml` at the repo root (`org.apache.storm.checkstyle.AutoFix`).
+2. **`src/main/resources/storm/checkstyle-autofix.groovy`** (via `gmavenplus-plugin`) -
+ runs Checkstyle itself, in-process, with the same version and ruleset the build
+ uses, reads violations (file, line, column, message) and hands each one to a small fixer written for the one
+ Checkstyle module that reported it. The cycle until no more rules can be fixed.
+
+The values a fixer needs (`Indentation` `basicOffset`, `LineLength` `max`,
+`CustomImportOrder` rules) are read from the ruleset XML at run time avoiding hardcoding. Files that the
+module excludes from Checkstyle (`<excludes>` of `maven-checkstyle-plugin`, e.g. thrift
+generated code) are neither audited nor rewritten.
+
+The license comment at the top of a file is never touched, whatever Checkstyle reports in it
+(`LineLength` on a wide-wrapped header, `JavadocParagraph` on one opened with `/**`): both
+wrappings of the ASF header are accepted, see [License headers](#license-headers).
Review Comment:
Thanks, verified locally on `storm-submit-tools` at 25327fbc: the headers are left alone now
Since both header wrappings are accepted, Checkstyle should stop reporting them too, otherwise every `/**` header keeps producing two `JavadocParagraph` warnings (lines 9 and 11) in the before/after reports of the workflow: 12 of the 20 remaining warnings on the module used for the test are exactly that, and it is the same on the other ~1450 files with that header.
A `SuppressionSingleFilter` in `storm_checkstyle.xml`, as a child of `Checker` right after the existing `SuppressionFilter`, does it:
```xml
<!-- https://checkstyle.org/filters/suppressionsinglefilter.html
Both wrappings of the ASF license header are accepted; the Javadoc one ('/**') trips
JavadocParagraph on its blank lines, so the check is muted on the header lines only. -->
<module name="SuppressionSingleFilter">
<property name="checks" value="JavadocParagraph"/>
<property name="lines" value="1-20"/>
</module>
```
Note for whoever tries it: the plugin reads the ruleset from the installed `storm-checkstyle` jar, so `mvn install -pl storm-checkstyle` is needed before the change shows up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org