Spotless / Checkstyle & Code Reformatting #14903

jdaugherty · 2025-07-14T03:45:09Z

Please see below comments on the impact of this PR. This PR will enable nohttp, code linting, and code validation for java files. Separately, we will apply groovy validations via code narc. Moreover, this PR does reformat the entire code base (non-test code) for the first time to have a consistent style.

jdaugherty · 2025-07-14T20:06:02Z

The style being applied is the style in grails-forge. It has this useful information on why this style was chosen:

Checkstyle configuration that checks the sun coding conventions from:

- the Java Language Specification at
  http://java.sun.com/docs/books/jls/second_edition/html/index.html

- the Sun Code Conventions at http://java.sun.com/docs/codeconv/

- the Javadoc guidelines at
  http://java.sun.com/j2se/javadoc/writingdoccomments/index.html

- the JDK Api documentation http://java.sun.com/j2se/docs/api/index.html

- some best practices

A couple pain points with this:

I'm only reformatting java & groovy production files. This change will only validate the java files - codenarc will do groovy and have to come after. Tests are being done in a later PR.
redundant declarations (final) are forced to be removed
ordering of the variable type matters (static comes before instance, public comes before private, etc)
inner class declarations must occur at the end of the file
inline assignments are discouraged
empty if statements are discouraged (at least add a comment)
static, final variables need to follow the upper case convention
accessors are supposed to exist for any public field on a java class (although , I've turned off check style for anything that would be considered a breaking change)
the JLS specifies the order of modifiers - public, abstract, static, final, etc - and they seem to be wrong in the grails code base
the older array syntax needs updated - ie char myVar[] vs char[] myVar ... did anyone else forget Java originally supported this?
javadoc has to be formatted correctly - when having code in javadoc, you still have to have the asterisk on each line or the reformat will collapse it to a single line.
I'm converting wildcard to single imports per previous discussion
duplicate lines seem to exist throughout the project (historical merges, etc). These have to be removed.
If a comment that is unrelated to licensing existed and doesn't exist now, I've removed it rather than have a broken link in the code as part of the https validation.

grails-gsp/core/src/main/groovy/org/grails/gsp/io/DefaultGroovyPageLocator.java

jdaugherty · 2025-07-15T02:17:41Z

#13754

borinquenkid

Instead of failing the build for checkstyle, I would suggest putting a githook on prepush to apply spotless
` #!/bin/sh

echo "[git hook] Running Spotless apply before push..."

# Run spotlessApply
./gradlew spotlessApply

# Check if there are any changes after spotlessApply
if ! git diff --quiet; then
    echo "Spotless applied changes. Creating a separate commit..."
    git add .
    git commit -m "chore: Apply Spotless formatting" --no-verify # --no-verify to prevent recursion
fi

# Proceed with the original push
exit 0`

I have only used it on maven, so I can't suggest a gradle plugin

jdaugherty · 2025-07-16T04:56:56Z

Instead of failing the build for checkstyle, I would suggest putting a githook on prepush to apply spotless ` #!/bin/sh
echo "[git hook] Running Spotless apply before push..."

# Run spotlessApply
./gradlew spotlessApply

# Check if there are any changes after spotlessApply
if ! git diff --quiet; then
    echo "Spotless applied changes. Creating a separate commit..."
    git add .
    git commit -m "chore: Apply Spotless formatting" --no-verify # --no-verify to prevent recursion
fi

# Proceed with the original push
exit 0`
I have only used it on maven, so I can't suggest a gradle plugin

I think it's reasonable to not fail the build on styling, but I do think we need a Github workflow that does fail if the styling is wrong. A prehook would be one way to help avoid issues, but it won't avoid all of them - some of the checkstyle configuration isn't fixable by spotless. For example, checkstyle mandates variable declarations be in certain order by their access level & classes be defined at the end of a file.

What are your thoughts about this:

suggest the prehook for formatting purposes
add a build that exclusively runs the checkstyle / linting (if linting shows anything changed, we would fail too)?

You can then iterate locally and fix after the fact.

jamesfredley · 2025-07-16T22:33:18Z

https://desktop.github.com/download/ is a good option for reviewing this PR. There are too many changes for the browser and it has same unified and split view options.

codeconsole · 2025-07-16T23:31:50Z

Nice work.

Hard to view the change set because there are so many files and it crashes my browser.

What is the reason for annotation parameters having a space? That seems a little weird, no?

@DelegateAsync (BookApi)

jdaugherty · 2025-07-16T23:34:27Z

Nice work.

Hard to view the change set because there are so many files and it crashes my browser.

What is the reason for annotation parameters having a space? That seems a little weird, no?
@DelegateAsync (BookApi)

That is odd. All I did here was format / spotless apply / checkstyle and then fix the remaining errors for checkstyle rules that forge had defined. If someone can suggest changes to the configuration, we can easily reformat the 2 commits that were styling related.

jamesfredley · 2025-07-17T00:04:54Z

DelegateAsync (BookApi) with the space appears to only occur in Groovydoc

* @DelegateAsync (BookApi)

jamesfredley

In Converter.java I don't think we want to remove the public modifiers, since it is a java file. This is the only java file, I found, where this occurred.

We have a mix of

try {
}
catch (..) {
}

and

try {
} catch (..) {
}

with only some files changed in this PR. Our checkstyle config should enforce the later based on https://checkstyle.sourceforge.io/checks/blocks/rightcurly.html

The following may not be relevant for this PR, but I tripped over it while looking into try/catch formatting.

The config copied from grails-forge appears to be using com.puppycrawl.tools:checkstyle:8.33 vs 10.26.1. https://docs.gradle.org/current/userguide/checkstyle_plugin.html

And the spotless config appears to only be checking the license headers, since we have not applied a formatter.
https://github.com/diffplug/spotless/tree/main/plugin-gradle#java

matrei

I have not gone through all files, but I found enough issues to start discussion/iteration.

Disclaimer: I have not used Checkstyle or Spotless before.

gradle/style-enforcement-config.gradle

matrei · 2025-07-17T07:42:29Z

gradle/style-enforcement-config.gradle

+checkstyle {
+    configFile = project.rootProject.layout.projectDirectory.file('etc/config/checkstyle/checkstyle.xml').asFile
+    configDirectory = project.rootProject.layout.projectDirectory.dir('etc/config/checkstyle').asFile
+
+    // Per submodule
+    maxErrors = 1
+    maxWarnings = 10
+
+    showViolations = true
+}


Suggestion:

extensions.configure(CheckstyleExtension) { // Explicit `it` required in extension configuration it.configFile = rootProject.layout.projectDirectory.file('etc/config/checkstyle/checkstyle.xml').asFile it.configDirectory = project.rootProject.layout.projectDirectory.dir('etc/config/checkstyle').asFile it.maxErrors = 1 it.maxWarnings = 10 it.showViolations = true }

gradle/style-enforcement-config.gradle

matrei · 2025-07-17T07:51:38Z

gradle/style-enforcement-config.gradle

+if (tasks.names.contains('checkstyleTest')) {
+    tasks.named('checkstyleTest').configure {
+        it.group = 'verification'
+        it.enabled = false


Add comment why we have chosen to disable checkstyleTest?

grails-async/core/src/main/groovy/grails/async/DelegateAsync.groovy

matrei · 2025-07-17T11:43:51Z