Skip to content

Fix Checkstyle errors and improve code quality (final parameters, lin… #1142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
singghh wants to merge 2 commits into from
Closed

Fix Checkstyle errors and improve code quality (final parameters, lin… #1142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f9b99d
Fix Checkstyle errors and improve code quality (final parameters, lin…
singghh Mar 25, 2025
c110bca
Fixed Checkstyle issues and added package-info.java
singghh Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Fixed Checkstyle issues and added package-info.java
  • Loading branch information
@singghh
singghh committed Mar 25, 2025
commit c110bcaced36ea26c97c1407c628b7025f6912a7
53 changes: 52 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,5 +177,56 @@
<url>https://repo.jenkins-ci.org/public/</url>
</pluginRepository>
</pluginRepositories>

<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.1.1</version>
Copy link
Contributor

@MarkEWaite MarkEWaite Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the most recent release of the checkstyle plugin.

Suggested change
<version>3.1.1</version>
<version>3.6.0</version>

Copy link
Author

@singghh singghh Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MarkEWaite MarkEWaite Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<executions>
<execution>
<phase>validate</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
</plugin>
<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>
Comment on lines +195 to +206
Copy link
Contributor

@MarkEWaite MarkEWaite Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, since it is already provided by the parent pom.

Suggested change
<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>
Comment on lines +207 to +212
Copy link
Contributor

@MarkEWaite MarkEWaite Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already provided by the parent pom.

Suggested change
<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>
Comment on lines +213 to +229
Copy link
Contributor

@MarkEWaite MarkEWaite Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. Provided by the parent pom.

Suggested change
<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>

</plugins>
</build>
</project>
4 changes: 2 additions & 2 deletions src/main/java/io/jenkins/docker/DockerTransientNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public DockerComputer createComputer() {

@Nullable
@Override
public ProvisioningActivity.Id getId() {
public final ProvisioningActivity.Id getId() {
return provisioningId;
}

Expand All @@ -184,7 +184,7 @@ private interface ILogger {

@Override
@Restricted(NoExternalUse.class)
public void _terminate(final TaskListener listener) {
public final void _terminate(final TaskListener listener) {
final ILogger tl = createILoggerForTaskListener(listener);
try {
terminate(tl);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/jenkins/docker/connector/DockerComputerAttachConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ enum ArgumentVariables {
private final String name;
private final String description;

ArgumentVariables(String name, String description) {
ArgumentVariables(final String name, final String description) {
this.name = name;
this.description = description;
}
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/io/jenkins/docker/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* This package contains classes related to the Jenkins pipeline integration
* for Docker, including the management of Docker nodes and steps within the pipeline.
*
* <p>The classes in this package manage Docker pipeline steps, node provisioning,
* and the execution of build jobs within Docker containers.</p>
*/
package io.jenkins.docker;
4 changes: 2 additions & 2 deletions src/main/java/io/jenkins/docker/pipeline/DockerNodeStepExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ private static DockerAPI defaultApi() {
"Must either specify dockerHost/credentialsId, or define at least one Docker cloud");
}

private void invokeBody(DockerTransientNode node, TaskListener listener) {
private void invokeBody(final DockerTransientNode node, final TaskListener listener) {
this.nodeName = node.getNodeName();
FilePath ws = null;
Computer computer = null;
Expand Down Expand Up @@ -256,7 +256,7 @@ public void stop(@NonNull Throwable cause) throws Exception {
private static class Callback extends BodyExecutionCallback.TailCall {
private final String nodeName;

public Callback(Node node) {
Callback(Node node) {
this.nodeName = node.getNodeName();
}

Expand Down
Loading