Fix Checkstyle errors and improve code quality (final parameters, lin… #1142
Conversation
…e length, and final class declaration)
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
Please provide a detailed description of how you tested these changes interactively. The pull request template specifically notes that you need to describe the tests that validate the changes are working. Running mvn checkstyle:checkstyle is not enough.
Fixed Checkstyle errors related to parameter finalization and line length.
Please don't attempt to fix checkstyle complaints about line length. The spotless formatter handles line length automatically.
I'm not yet persuaded that adding checkstyle and resolving its complaints add enough value to the plugin. The automated code formatting from spotless already provides much of the value
| <plugin> | ||
| <groupId>com.github.spotbugs</groupId> | ||
| <artifactId>spotbugs-maven-plugin</artifactId> | ||
| <version>4.2.0</version> | ||
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>check</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
Not needed, since it is already provided by the parent pom.
| <plugin> | |
| <groupId>com.github.spotbugs</groupId> | |
| <artifactId>spotbugs-maven-plugin</artifactId> | |
| <version>4.2.0</version> | |
| <executions> | |
| <execution> | |
| <goals> | |
| <goal>check</goal> | |
| </goals> | |
| </execution> | |
| </executions> | |
| </plugin> |
| <plugin> | ||
| <groupId>org.jenkins-ci.tools</groupId> | ||
| <artifactId>maven-hpi-plugin</artifactId> | ||
| <version>3.37</version> <!-- Ensure this is the latest stable version --> | ||
| <extensions>true</extensions> | ||
| </plugin> |
There was a problem hiding this comment.
Already provided by the parent pom.
| <plugin> | |
| <groupId>org.jenkins-ci.tools</groupId> | |
| <artifactId>maven-hpi-plugin</artifactId> | |
| <version>3.37</version> <!-- Ensure this is the latest stable version --> | |
| <extensions>true</extensions> | |
| </plugin> |
| <plugin> | ||
| <groupId>com.diffplug.spotless</groupId> | ||
| <artifactId>spotless-maven-plugin</artifactId> | ||
| <version>2.17.0</version> | ||
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>apply</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| <configuration> | ||
| <java> | ||
| <googleJavaFormat /> | ||
| </java> | ||
| </configuration> | ||
| </plugin> |
There was a problem hiding this comment.
Not needed. Provided by the parent pom.
| <plugin> | |
| <groupId>com.diffplug.spotless</groupId> | |
| <artifactId>spotless-maven-plugin</artifactId> | |
| <version>2.17.0</version> | |
| <executions> | |
| <execution> | |
| <goals> | |
| <goal>apply</goal> | |
| </goals> | |
| </execution> | |
| </executions> | |
| <configuration> | |
| <java> | |
| <googleJavaFormat /> | |
| </java> | |
| </configuration> | |
| </plugin> |
| if (cloud != null) { | ||
| final String uuid = UUID.randomUUID().toString(); | ||
| final DockerTemplate template = jobTemplate.getTemplate().cloneWithLabel(uuid); | ||
| final DockerTemplate template = |
There was a problem hiding this comment.
Please use mvn spotless:apply rather than performing manual formatting of the files. The Jenkins plugin parent pom provides spotless with a configuration that works well for Jenkins plugins. It is enabled in this plugin.
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-checkstyle-plugin</artifactId> | ||
| <version>3.1.1</version> |
There was a problem hiding this comment.
Please use the most recent release of the checkstyle plugin.
| <version>3.1.1</version> | |
| <version>3.6.0</version> |
There was a problem hiding this comment.
Thank you for your previous comments and guidance,
I noticed that we don't currently have a checkstyle.xml file in the repository or a clear indication of where it might be referenced. I'm wondering if there is a plan to introduce this file, or if we're using an external/shared configuration that I might have overlooked? I ran mvn spotless:apply successfully, but just wanted to make sure we're not missing any custom style rules that might come with Checkstyle.
There was a problem hiding this comment.
I'm wondering if there is a plan to introduce this file, or if we're using an external/shared configuration that I might have overlooked? I ran mvn spotless:apply successfully, but just wanted to make sure we're not missing any custom style rules that might come with Checkstyle.
I'm not planning to introduce checkstyle into this repository. The Jenkins plugins that I maintain do not use checkstyle. I've not found it to be valuable enough to be worth the effort to enable it.
If you're serious about enabling checkstyle in this repository, then you'll need to provide a complete implementation that includes the checkstyle settings to persuade me that it will be useful. The default checkstyle settings conflict with spotless formatting. I value spotless formatting much more than I value checkstyle warnings. If your pull request proposes to accept many new, unresolved checkstyle warnings, then I'll reject it. I'm not willing to add new warnings output because it will be a distraction and will create extra work for me to resolve the warnings.
|
@singghh you should confirm locally that if you decide to continue with this pull request, I'll rely on you to mark it as "ready for review" when you've completed the changes that allow it to pass If you'd like to see a reference implementation of checkstyle checks for a Jenkins plugin, refer to: |
|
Closing after 6 weeks with no response. Can be reopened if responses are provided in the future. |
2 participants
Description
This pull request addresses the following:
DockerTemplateLabelAssignmentActionas final to meet Checkstyle requirements.Changes
wi,li, anditemhave been marked asfinal.DockerTemplateLabelAssignmentActionhas been made final.How to Test
Run the Checkstyle tool to verify that all errors have been fixed.
Checklist