Improve checkstyle performance and readability #53925
Conversation
Drop a nasty regex in our checkstyle config that I wrote a long time ago in favor of a checkstyle extension. This is better because: * It is faster. It saves a little more than a minute across the entire build. * It is easier to read. Who knew 100 lines of Java would be easier to read than a regex, but it is. * It has tests.
nik9000
added
>enhancement
:Delivery/Build
| @@ -0,0 +1,503 @@ | |||
| GNU LESSER GENERAL PUBLIC LICENSE | |||
There was a problem hiding this comment.
@tomcallahan could you take a look at this? We depend on checkstyle in the build and this extension is part of the build.
| @@ -8,6 +8,7 @@ rootProject.name = dirName | |||
| List projects = [ | |||
| 'build-tools', | |||
| 'build-tools:reaper', | |||
| 'checkstyle', | |||
There was a problem hiding this comment.
I did not put this in buildsrc because it looks like the logger usage check to me. I can certainly move it if you'd feel more comfortable with it in the buildSrc but I might need tips on how to reference it in that case.
There was a problem hiding this comment.
It probably should move as well. It's not production code, but the current configuration is probably just historical.
|
Please do put the project under buildSrc. We should arrive to have anything build related under there, even in subprojects like reaper. |
| continue; | ||
| } | ||
| if (false == line.startsWith(leadingSpaces)) { | ||
| log.accept(lines.lastLineNumber, "snippet line should start with [" + leadingSpaces + "]"); |
There was a problem hiding this comment.
This log message might be a little confusing as we are just going to print n spaces. Perhaps we should reword this to say that "snippet line should start with n leading spaces" to make it clear that we are talking indent width.
| * the page. | ||
| */ | ||
| public class SnippetLengthCheck extends AbstractFileSetCheck { | ||
| private static final Pattern START = Pattern.compile("^( *)//\\s*tag::(.+?)\\s*$", Pattern.MULTILINE); |
There was a problem hiding this comment.
We are we using multiline matchers when we are always matching on individual lines?
| @Override | ||
| @SuppressForbidden(reason = "Checkstyle's API contain File") | ||
| protected void processFiltered(File file, FileText fileText) throws CheckstyleException { | ||
| checkFile((line, message) -> log(line, message), max, fileText.toLinesArray()); |
There was a problem hiding this comment.
Seems a bit unnecessarily to plum this log function which just maps 1:1 with the log() method of the parent class. Is this just so that these methods can be static? Is there any real benefit to that? Does checkstyle create multiple instances of this?
There was a problem hiding this comment.
It means I don't have to figure out anything about the way checkstyle works to write the test. I could probably rework it so the test is more checkstyle-native if you'd prefer.
|
Update for those playing along at home: @mark-vieira is going to see if he can find a nice way to move the new project into |
|
This PR is superseded by #54308. |
Drop a nasty regex in our checkstyle config that I wrote a long time ago
in favor of a checkstyle extension. This is better because:
build.
read than a regex, but it is.
Closes #53921