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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
