rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

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

Open yeikel opened this issue 4 years ago • 8 comments

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 :)

yeikel avatar Apr 08 '22 05:04 yeikel

Three forms exist in an initial implementation:

  1. Skip by name image

  2. Skip by class image

  3. Skip all image

jkschneider avatar Apr 11 '22 19:04 jkschneider

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

yeikel avatar Apr 12 '22 18:04 yeikel

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.

traceyyoshima avatar Apr 28 '22 22:04 traceyyoshima

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

yeikel avatar Apr 28 '22 23:04 yeikel

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.

traceyyoshima avatar Apr 28 '22 23:04 traceyyoshima

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

yeikel avatar Apr 28 '22 23:04 yeikel

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.

traceyyoshima avatar Apr 29 '22 00:04 traceyyoshima

@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.

jkschneider avatar Apr 29 '22 19:04 jkschneider

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.

knutwannheden avatar Nov 04 '22 04:11 knutwannheden

@knutwannheden There is a visitor called HasTypeOnClasspathSourceSet currently.

jkschneider avatar Nov 10 '22 12:11 jkschneider