openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.27k stars 338 forks source link

Add dynamic applicable tests that prevent a recipe from running on a particular class. #1596

Closed yeikel closed 2 years ago

yeikel commented 2 years ago

Is there any way to ignore specific recipes from a line or class?

For example, when I faced https://github.com/openrewrite/rewrite/issues/1478 and https://github.com/openrewrite/rewrite/issues/1536, I had to disable the individual recipes from my entire build until it was fixed. It would have been better if I had a way to disable that specific instance instead

Another example : currently, https://github.com/openrewrite/rewrite-testing-frameworks/issues/201 forced me to disable org.openrewrite.java.testing.cleanup.BestPractices for all my projects until it is fixed

In my particular example, I have the rewrite configuration in a parent pom that is inherited by all my projects, if any of my individual projects encounters any rewrite bug, I'd need to create a release or overwrite the entire rewrite configuration in the individual module. Both of these options are not desirable

There are also other scenarios where I simply do not want to run specific recipes for specific classes (For example org.openrewrite.java.cleanup.FinalClass )

If it helps in any way, in Sonar we have

//NOSONAR and also @SuppressWarnings("squid:S2078")

In an ideal world, this inline functionality should be easily removable with another recipe :)

jkschneider commented 2 years ago

Three forms exist in an initial implementation:

  1. Skip by name

    image
  2. Skip by class

    image
  3. Skip all

    image
yeikel commented 2 years ago

Thank you. This is a great start

I would like to have the option to disable specific lines as well so if you don't mind let's leave the issue open until that's feasible

traceyyoshima commented 2 years ago

Hi @yeikel, I spoke with the team and we won't be pursuing exclusions. Info on some of the complexities may be found here.

We'd like to improve our style detection so that recipes won't execute if there are additional configurations like //NOSONAR, @SuppressWarnings("squid:S2078"), check-style configurations, and/or @RewriteSkip.

Happy to fix bugs if anything else comes up, please let me know if you have any questions.

yeikel commented 2 years ago

Hi @yeikel, I spoke with the team and we won't be pursuing exclusions. Info on some of the complexities may be found here.

We'd like to improve our style detection so that recipes won't execute if there are additional configurations like //NOSONAR, @SuppressWarnings("squid:S2078"), check-style configurations, and/or @RewriteSkip.

Happy to fix bugs if anything else comes up, please let me know if you have any questions.

This is unfortunate but I understand that it is complex

My reasoning is not only for bugs but also for personal preference

Without a mechanism to have exclusions, I'll have to revert the recipe application with each execution, and that's quite tedious and error prone to be honest

traceyyoshima commented 2 years ago

Ah, I see.

Without a mechanism to have exclusions, I'll have to revert the recipe application with each execution, and that's quite tedious and error prone to be honest

A potential workaround would be to automate adding the @RewriteSkip(...) annotation to specific files via a recipe. The annotation would prevent unwanted executions. Then a separate recipe may remove the annotations afterward.

yeikel commented 2 years ago

Ah, I see.

Without a mechanism to have exclusions, I'll have to revert the recipe application with each execution, and that's quite tedious and error prone to be honest

A potential workaround would be to automate adding the @RewriteSkip(...) annotation to specific files via a recipe. The annotation would prevent unwanted executions. Then a separate recipe may remove the annotations afterward.

I believe that @RewriteSkip even if manual should do the job for me but it would be nice to have some way to automate its removal

traceyyoshima commented 2 years ago

but it would be nice to have some way to automate its removal

Ya, an internal recipe may take in a set of class FQNs like AddRewriteSkip and RemoveRewriteSkip. In visitClassDecl the class FQN may be checked if it's contained in the specified list. Then use the java template to add the annotation.

A similar process may be used to remove the annotation automatically as well.

jkschneider commented 2 years ago

@traceyyoshima reopening because the mechanism does not exist yet to apply this functionality. Likely the best option right now is to enhance YAML configuration of a recipe to allow the application of dynamic applicable tests. Something like this:

image

cc / @yeikel

Maybe it should be called if, applicableTest to align it the the programmatic form, something else entirely? The question will naturally come up about whether there should be boolean operations on these things (after all every recipe in the catalog could be either a positive or negative signal of applicability depending on the circumstances):

---
type: specs.openrewrite.org/v1beta/recipe
name: io.moderne.CleanupSkipAntlr
displayName: Code cleanup
description: Automatically cleanup code, e.g. remove unnecessary parentheses, simplify expressions.
applicableIf:
  - not:
    org.openrewrite.HasSourcePath:
      syntax: glob
      fileMatcher: ./rewrite-java/src/main/gen/**
recipeList:
  - org.openrewrite.java.cleanup.Cleanup

I would avoid these kinds of complications for now.

knutwannheden commented 2 years ago

This looks interesting. There could then possibly also be a predicate to check for a given class or possibly even Maven module on the classpath.

jkschneider commented 2 years ago

@knutwannheden There is a visitor called HasTypeOnClasspathSourceSet currently.