[POC] Modernize codebase - Automatically fix Checkstyle violations

[Pankraz76]

Modernize codebase - Automatically fix Checkstyle violations - implement IDE agnostic configuration with editorconfig.org

ref:

• https://docs.openrewrite.org/running-recipes/popular-recipe-guides/automatically-fix-checkstyle-violations
• Pull apache#2301: Modernize codebase - Bump maven-checkstyle-plugin to: 3.6.0 #2301
• [POC] editorconfig-maven-plugin: implement IDE agnostic configuration #2321
• Issue #16543: implement IDE agnostic configuration with editorconfig.org checkstyle/checkstyle#16574

[gnodet]

Ah, now, you enable the tooling. That's becoming interesting...

[olamy]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

[gnodet]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

We still use checkstyle for things other than code formatting.

[gnodet]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

We still use checkstyle for things other than code formatting.

But I'm quite bored with rules that fail. I'd much rather have tooling that fixes things automatically (or at least is able to do so, same as spotless).

[gnodet]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

We still use checkstyle for things other than code formatting.

But I'm quite bored with rules that fail. I'd much rather have tooling that fixes things automatically (or at least is able to do so, same as spotless).

So the workflow would rather be: run openrewrite rules and make sure there's no changes.

[olamy]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

We still use checkstyle for things other than code formatting.

But I'm quite bored with rules that fail. I'd much rather have tooling that fixes things automatically (or at least is able to do so, same as spotless).

So the workflow would rather be: run openrewrite rules and make sure there's no changes.

Right. So it might be better to have this in the maven-parent?

[gnodet]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

We still use checkstyle for things other than code formatting.

But I'm quite bored with rules that fail. I'd much rather have tooling that fixes things automatically (or at least is able to do so, same as spotless).

So the workflow would rather be: run openrewrite rules and make sure there's no changes.

Right. So it might be better to have this in the maven-parent?

@Pankraz76 do you want to focus on that, rather than not-enforced checkstyle rules and results of openrewrite recipes ?

[Pankraz76]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

We still use checkstyle for things other than code formatting.

But I'm quite bored with rules that fail. I'd much rather have tooling that fixes things automatically (or at least is able to do so, same as spotless).

So the workflow would rather be: run openrewrite rules and make sure there's no changes.

Right. So it might be better to have this in the maven-parent?

@Pankraz76 do you want to focus on that, rather than not-enforced checkstyle rules and results of openrewrite recipes ?

yes good idea. Thanks

[Pankraz76]

already for the code

yes its very nice, but still not fully automated, kind of demanding .editorconfig, to fix issues before they occur, thus promote the nice tool spotless, to an second layer:

• Issue #16543: implement IDE agnostic configuration with editorconfig.org checkstyle/checkstyle#16574

Editorconfig has autofix as well but not for custom IDE specs like this tool really plays out its cards.

Just giving you my noob exp upfront. Its fixing itself, like magic, but not for imports, failing the build.

ij_java_names_count_to_use_import_on_demand = 999

this issue unsolved to me. 1 LOC will fix for all: ij_java_names_count_to_use_import_on_demand = 999

[gnodet]

already for the code

yes its very nice, but still not fully automated, kind of demanding .editorconfig, to fix issues before they occur, thus promote the nice tool spotless, to an second layer:

• Issue #16543: implement IDE agnostic configuration with editorconfig.org checkstyle/checkstyle#16574

Editorconfig has autofix as well but not for custom IDE specs like this tool really plays out its cards.

Just giving you my noob exp upfront. Its fixing itself, like magic, but not for imports, failing the build.

ij_java_names_count_to_use_import_on_demand = 999

Integration in IDE is nice to have, but more importantly refactoring at build time using a maven plugin in a profile.

[Pankraz76]

run

Would be nice to have it in place running like rewrite, via github actions:

openrewrite/rewrite-static-analysis#497 (comment)

Its nifty that we have outdated stuff gone in

• Remove unused private methods #2310

, but its not banned and fixed on the long run; still causing friction, needed to be automated.

As local run is optional, but the way to do. We would add all wanted styles and make like spotless. Then after clean install code would be fixed, but not forces to be pushed again.

Checkstyle will ensure spotless violations, but not rewrite recipes (e.g. unused methods). Would need to add rewrite checks and auto apply them, or kind of prevent merge, until resolved to solve this. feels wierd.

Whats your advice, how to fix this on the long run? @timtebeek

• https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#unusedprivatemethod
• https://docs.openrewrite.org/recipes/staticanalysis/removeunusedprivatemethods

[gnodet]

run

Would be nice to have it in place running like rewrite, via github actions:

openrewrite/rewrite-static-analysis#497 (comment)

Its nifty that we have outdated stuff gone in

• Remove unused private methods #2310

, but its not banned and fixed on the long run; still causing friction, needed to be automated.

As local run is optional, but the way to do. We would add all wanted styles and make like spotless. Then after clean install code would be fixed, but not forces to be pushed again.

Checkstyle will ensure spotless violations, but not rewrite recipes (e.g. unused methods). Would need to add rewrite checks and auto apply them, or kind of prevent merge, until resolved to solve this. feels wierd.

Whats your advice, how to fix this on the long run? @timtebeek

• https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#unusedprivatemethod

• https://docs.openrewrite.org/recipes/staticanalysis/removeunusedprivatemethods

I don't think changing the code in an action would be a good idea. I'd really favor the same behavior we use with spotless, i.e. a profile (activated by default) which fails the build if a rule cannot be validated (in the case of open rewrite, if a rule would actually apply and change the code) and a profile which would actually apply the rules.

[Pankraz76]

Not sure I understand here. As we are using spotless already for the code, what is this PR providing?

might be not configured nor supported; this seems to be a gap in project setup, as despite nice tooling, still having misaligned code:

assuming our project dont require spiked line endings like PMD and Check to pass build.

• https://www.aleksandrhovhannisyan.com/blog/crlf-vs-lf-normalizing-line-endings-in-git/
• https://labex.io/questions/how-to-ensure-consistency-of-line-endings-when-collaborating-on-a-project-12650#:~:text=When%20collaborating%20on%20a%20project%20using%20Git%2C%20it's%20crucial%20to,line%20in%20a%20text%20file.

#2321 (comment)

[gnodet]

The OpenRewrite maven plugin seems a much better way to fix the problems imho:
https://docs.openrewrite.org/reference/rewrite-maven-plugin

The rewrite:dryRun goal with <failOnDryRunResults>true</failOnDryRunResults> directly addresses the need by checking for potential changes and failing the build if any are found, giving confidence that the codebase is in the desired state.

@Pankraz76 could you please close all the PRs about refactoring that use openrewrite and focus on adding the plugin in the build instead ?

[Pankraz76]

actually done:

• [POC] rewrite-maven-plugin: Introduce OpenRewrite by Moderne #2322

failOnDryRunResults is what we need. Thanks

[slawekjaranowski]

we already have a checkstyle configuration in parent pom ..... and rules have a different source

[Pankraz76]

successor: #2322

[timtebeek]

At OpenRewrite, and others like Langchain4j, we run recipes that suggest fix comments on PRs as documented here:https://www.moderne.ai/blog/stop-breaking-ci-annotate-prs-with-openrewrite-recipe-fixes-as-quality-gate
Happy to help set that up in a couple weeks when in back from my travels. I think those code suggestions on PRs are more helpful than breaking the build.

[Pankraz76]

suggestions

we need strict leadership on our suggestions enforcing them like good cop, bad cop.

[Pankraz76]

travels

enjoy