Improve baseline-checkstyle implementation and upgrade to 8.12 #313
Conversation
| project.getTasks().withType(Javadoc.class, javadoc -> { | ||
| if (project.getConvention().getPlugin(JavaPluginConvention.class) | ||
| .getSourceCompatibility().isJava8Compatible()) { | ||
| javadoc.options(minimalJavadocOptions -> ((StandardJavadocDocletOptions) minimalJavadocOptions) |
There was a problem hiding this comment.
this is really annoying but looks like the api is a little bs here
There was a problem hiding this comment.
If the if statement is always going to return the same value, could we move it outside the withType(Javadoc.class, -> bit?
| project.getTasks().withType(Checkstyle.class, checkstyle -> { | ||
| // Make checkstyle include files in src/main/resources and src/test/resources, e.g., for whitespace checks. | ||
| checkstyle.source("src/main/resources"); | ||
| checkstyle.source("src/test/resources"); |
There was a problem hiding this comment.
Is it worth iterating through sourcesets here? Repos commonly add integTest too
There was a problem hiding this comment.
there's like an ordering problem here ...
| */ | ||
| public final class BaselineCheckstyle extends AbstractBaselinePlugin { | ||
|
|
||
| private static final String DEFAULT_CHECKSTYLE_VERSION = "8.11"; |
There was a problem hiding this comment.
I guess we should probably cut and RC so that we can try out the 7.5.1 -> 8.11 upgrade on a few internal projects?
There was a problem hiding this comment.
want to go straight to 8.12?
|
So just to double check, this PR is essentially:
|
|
Yes, I am trying to go through some of the plugins here and make them more modern/conforming to current practices. The checkstyle bump is here since in #205 we expressed desire to do so and we never did overall and current version is 1.5 year old now |
|
Repeating my question from #205 : There's a pretty big change list between 7.5.1 and 8.1. Did you go through them and verify that (1) we're comfortable with the changes, (2) they align with our Baseline docs, (3) they are compatible with the IntelliJ and Eclipse formatting, (4) they require no other config change? |
| .setConfigDir(project.file(Paths.get(getConfigDir(), "checkstyle").toString())); | ||
| project.getTasks().withType(Checkstyle.class, checkstyle -> { | ||
| // Make checkstyle include files in src/main/resources and src/test/resources, e.g., for whitespace checks. | ||
| project.getConvention().getPlugin(JavaPluginConvention.class).getSourceSets() |
There was a problem hiding this comment.
need to verify this is the correct path
There was a problem hiding this comment.
It is the correct path though it's impossible to test in unit tests. Will add an integration test
|
Run this locally - need to update some checkstyle config to match new values. Things I found that are different: |
|
The DFE thing is only because the repo I looked at is using junit5 and defaults don't account for it |
robert3005
force-pushed
the
rk/checkstyle
branch
2 times, most recently
from
e4300cd
to
62131be
Compare
|
After adjusting the checkstyle config to match what we expected before I get only |
robert3005
force-pushed
the
rk/checkstyle
branch
from
29a11c7
to
74fdd0d
Compare
robert3005
force-pushed
the
rk/checkstyle
branch
from
9994160
to
c1c539b
Compare
| @@ -433,6 +433,10 @@ | |||
| <property name="format" value="^[a-z][a-zA-Z0-9]+$"/> | |||
| <message key="name.invalidPattern" value="Parameter name ''{0}'' must match pattern ''{1}''."/> | |||
| </module> | |||
| <module name="LambdaParameterName"> <!-- Java Style Guide: Parameter names --> | |||
There was a problem hiding this comment.
Adds parameter name checkstyle for lambda parameters. There's issues with ParameterName right now for lambdas that it doesn't correctly detect lambdas (and after applying this I can confirm that I have few violations of lambda parameter names that weren't caught before). From 8.12 checkstyle ParameterName will completely ignore lambda parameters and this check is preferred.
|
@uschi2000 how do you feel about the current state? |
|
Looks good, thanks @robert3005 . Can you include the 8.11/8.12 bump in the commit message, please? |
robert3005
force-pushed
the
rk/checkstyle
branch
from
4093332
to
9367a74
Compare
robert3005
changed the title
|
We're going to remove LambdaParameterName from the rules since in practice we found way too many violations for this rule to be meaningful |
@iamdanfox
fixes #207
fixes #198