Reformat code

[matrei]

Rules can be introduced gradually, and source files reformatted incrementally to maintain a manageable review process and avoid overwhelming diffs.

Formatting commits will be squashed before merging to minimize noise in the project history.

Related: #14903

[jamesfredley]

@matrei I resolved the merge conflicts with 7.0.x after #14988. I overlooked the single quotes when resolving the conflicts, but pushed another commit to change them back.

[jamesfredley]

I resolved the merge conflicts after #15003

[jamesfredley]

@matrei Thank you. This makes a massive improvement to the codebase and I am onboard with the format decision you have selected.

How do you propose we keep in sync with these rules and should we apply formatting from the IDE or a Gradle task?

[jdaugherty]

This appears to be an excellent, gradual approach to code style enforcement. Great Job @matrei!

Some concerns:

1. Auto-formatting. I don't see an intellij configuration checked in, editorconfig, or spotless. Do you have a way to auto format to prevent or minimize styling errors?

2. Clear indication of why the Github action fails. Can we configure the code style validations to have their own Github action & then disable them in the main test runs? My concern is we won't be able to tell what's broken and we can often fix styling issues in parallel to fixing tests. This will also avoid slowing down test builds.

3. As part of #2, can we generate a single code narc report that's published as part of the code style build? Can the same be generated for checkstyle?

[matrei]

I think we should actually remove the Checkstyle and CodeNarc files from this PR and only squash the code formatting changes.
Then we add back Checkstyle, CodeNarc and code formatting helpers in another commit that is not in .git-blame-ignore-revs.
What do you think?

[jdaugherty]

I am ok with that, but do you agree with separating into a separate action so we can fix issues in parallel?

[matrei]

As the full build takes like 40 minutes, will it not actually save CI time/resources to fail early, before starting to run the test suites.
The tests will have to run again anyway. I think that is how the Spring repos handles it, it fails directly on formatting errors.

[jdaugherty]

The issue is fixing in parallel, so if it's only a code style error vs a real test error. This allows someone to fix the formatting issues after finding a solution to aprobelm / the tests.

I'm proposing to move the formatting errors to it's own build.

We also need to upload the errors somewhere - they currently reference an html file that can't be viewed if it's from an action.

[matrei]

This allows someone to fix the formatting issues after finding a solution to aprobelm / the tests.

But shouldn't one preferably run the build locally to test changes before pushing?

We also need to upload the errors somewhere - they currently reference an html file that can't be viewed if it's from an action.

Yes, any code style error reports should be uploaded to the summary page.

[jdaugherty]

This allows someone to fix the formatting issues after finding a solution to aprobelm / the tests.

But shouldn't one preferably run the build locally to test changes before pushing?

We also need to upload the errors somewhere - they currently reference an html file that can't be viewed if it's from an action.

Yes, any code style error reports should be uploaded to the summary page.

Yes, but third party contributors may not always do that. Having a separate run makes it clear why it's failing and if it's a functionality problem or a best practice/ style issue.

I also wish to use that same functionality locally at times - there have been many times where I'm debugging an issue and don't want to worry about code style locally until a solution has been found. I 100% agree we should eventually be compliant with the code style, but I want a way to opt out locally when testing or making large changes.

[matrei]

but I want a way to opt out locally when testing or making large changes.

Isn't that why there are separate test and check Gradle tasks?

[jdaugherty]

but I want a way to opt out locally when testing or making large changes.

Isn't that why there are separate test and check Gradle tasks?

It's my understanding that test only runs the unit tests.

[matrei]

but I want a way to opt out locally when testing or making large changes.

Isn't that why there are separate test and check Gradle tasks?

It's my understanding that test only runs the unit tests.

Yes, integration tests are a feature added by the Integration Test Grails Gradle Plugin. This is not wired to the test task but on the check task. So at present you will have to run ./gradlew test iT to run all tests.

[jdaugherty]

I suggest we remove the gradle codenarc / checkstyle config, update the ignored rev list, squash, and merge this PR so we have a starting point on styling. You've done amazing work on this PR and I think it's important to merge.

Concerning the workaround of ./gradlew test iT, I don't see how that's equivalent to running the build task. Build will run other tasks such as process resources & execute our gradle plugins because of the functional test apps. One of the reasons we left gradle plugins applied in the test apps was to ensure their logic is executed, but those tasks won't work if the styling isn't perfect since they aren't a part of an integration run. Styling enforcement then could act as a delay when developing locally.

Concerning turning on enforcement, I'm against this implementation because it will interfere during periods of rapid, local development. We also need to be able to easily identify when a PR fails for a style issue vs logic issue. Having a disable option allows both development styles & it could support a separate workflow to point out styling issues. While integrating prevents that flexibility so I don't see a downside to supporting both vs always enforcing. If someone prefers to enforce always, they can leave it enabled.

I also think we need an autoformat option - like spotless to minimize the styling rules impacts for new contributors & to reduce errors. Do we have a solution for that?

[matrei]

OK, so if I understand you correctly, you are suggesting:

./gradlew build = no code style checks
./gradlew check = no code style checks

Perhaps a ./gradlew codeStyle that only runs checkstyle* and codenarc* tasks?

A separate GitHub workflow for Code Style checks (that uploads the reports to its summary page).

[jdaugherty]

OK, so if I understand you correctly, you are suggesting:

./gradlew build = no code style checks

./gradlew check = no code style checks

Perhaps a ./gradlew codeStyle that only runs checkstyle* and codenarc* tasks?

A separate GitHub workflow for Code Style checks (that uploads the reports to its summary page).

I was thinking just a property that enables/disables it. By default, it would run always and then we could toggle locally when needed or in an action as needed.