Add rewrite support for errorprone.refasterrules.FileRulesRecipes

[Pankraz76]

refs:

• [Experimental] Add rewrite support for RemoveUnusedPrivateMethods & RemoveUnusedImports opensearch-project/OpenSearch#18791
• [Experimental] Add rewrite support for errorprone.refasterrules.StringRulesRecipes diffplug/spotless#2576
• MINOR: Cleanup Trogdor Module #20214
• https://error-prone.picnic.tech/refasterrules/StringRules/

The code seems to be out of sync considering the changes made when applying the config/convention:

(Assuming the version bump alone did not make these changes in the headers)

Also assuming that Java files currently have no header check besides Checkstyle. It seems considerable to have the Spot config in one place, giving it Separation of Concerns as well as Single Source of Truth.

[Pankraz76]

outdated dependencies.

[Pankraz76]

BUILD SUCCESSFUL in 38m 46s
374 actionable tasks: 90 executed, 284 up-to-date
➜  kafka git:(rewrite-) ./gradlew build -x test 

Kindly request your feedback. Thanks for consideration.

Is this is any good?

@m1a2st
@sjhajharia
@ravikalla
@apoorvmittal10
@CalvinConfluent
@chia7712
@frankvicky

[ravikalla]

The PR is marked as "[Experimental]" but makes changes that would affect all developers immediately. This includes modifying the main build configuration, adding new required tasks to the check lifecycle, enabling build caching globally, and changing existing tool configurations. For an experimental feature, I'd suggest using a separate build file or opt-in flag instead. It would also be helpful to create a feature branch for testing, provide a way to disable the rewrite checks, and add proper documentation about the experimental nature of these changes.

[ravikalla]

Both Spotless and Rewrite are configured to handle imports: Spotless has importOrder() configuration and Rewrite has RemoveUnusedImports recipe active.

This creates overlapping responsibilities and potential conflicts. Consider either:

1. Using only Spotless for formatting/imports and Rewrite for code cleanup
2. Disabling import handling in one of the tools

[Pankraz76]

Yes, thanks for pointing out.

Indeed, Rewrite has the ability to take over, making Spot and Check obsolete, considering the recipe:

• AutoFormat Recipe - OpenRewrite Docs

As a potential migration is too much for now, this PR is considered kind of PoC.

The import case is a good example - Rewrite has also bugs of course, but in this case seems to be stronger due to the type awareness.
Assuming Spot (which uses Google's formatter) is just a simple file parser like Checkstyle, it has a lot of blind spots in the unused imports theme again.

2. Disabling import handling in one of the tools

yes shifted from spot to rewrite demonstrating the leftovers.

• Using only Spotless for formatting/imports and Rewrite for code cleanup

yes thats the goal to later on check if spot is still necessary.

But first start small then continue.

[ravikalla]

Thanks for clarifying. I agree that starting small is better. Since you're demonstrating Rewrite's capabilities as a PoC, I suggest you to document in the PR description that this is exploring Rewrite as a potential Spotless replacement. For now, you can explicitly disable import handling in Spotless to avoid conflicts. Consider creating a follow-up issue to track the full migration evaluation.

[Pankraz76]

The PR is marked as "[Experimental]" but makes changes that would affect all developers immediately. This includes modifying the main build configuration, adding new required tasks to the check lifecycle, enabling build caching globally, and changing existing tool configurations. For an experimental feature, I'd suggest using a separate build file or opt-in flag instead. It would also be helpful to create a feature branch for testing, provide a way to disable the rewrite checks, and add proper documentation about the experimental nature of these changes.

Maybe [PoC] would reflect my intention better as this is just some suggestion offered.

It needs to be wanted and pulled by the dev team, as it's a conventional change, not designed to opt in, as the nature of this can be (once) invasive and is meant to sustain therefore be constantly checked against. Same approach like spot but more flexible.

I would suggest to make a decision about using the potential benefit this tool has to offer and then integrate it non-breaking/colliding with anything of course, making it ready (opt-in/out) to be removed afterwards again, if it turns out, not to be practical.

The import statement is a good starter and the removals as they should flag only once and non-critical stuff as overhead is normally obsolete, ready to be reconsidered and resolved.

As the task is meant to run in cloud only it's about time and stability, as the recipes are individual.
Only the dev teams choose which to leverage and apply.

[Pankraz76]

BUILD SUCCESSFUL in 12m 31s
133 actionable tasks: 133 executed
16:38:11: Execution finished 'rewriteRun'.