Creating "OR" preconditions instead of "AND" (#4003) does not actually work

[DidierLoiseau]

What's changed?

Adds 2 failing tests using declarative recipes as preconditions.

Based on Creating "OR" preconditions instead of "AND", you should be able to use a declarative recipe as a precondition to combine recipes into an “or” clause, but it does not actually work.

Upon debugging, it appears that DeclarativeRecipe does not actually override getVisitor(), so when DeclarativeRecipe.getRecipeList() calls getVisitor() on the declarative recipe used as precondition, it just returns noop().

What's your motivation?

Be able to re-use a configured recipe as precondition for other recipes.

Anything in particular you'd like reviewers to focus on?

• In Declarative Preconditions - or association #4003 this use-case was documented and later reported as not working.

Anyone you would like to review specifically?

#4003 involved a discussion mostly between @timtebeek and @nmck257 who might be interested.

Have you considered any alternatives or workarounds?

Currently the only alternative is to repeat the predicate configuration, using only imperative recipes. If you need a disjunction of conditions, you will have to duplicate the whole recipe, but it may not always work.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Very much appreciated! Had this in the back of my mind still, but didn't quite get to writing the tests and giving them a try. Indeed this ought to work as described, as that's a nice simplification where possible. Did you already explore what might be the cause? I wonder if the getRecipeList() is evaluated at all, or whether it's only looking at getVisitor()... 🤔

[DidierLoiseau]

Did you already explore what might be the cause? I wonder if the getRecipeList() is evaluated at all, or whether it's only looking at getVisitor()... 🤔

I did some debugging which I described in the "what changed" section. I don't know what would be an appropriate solution though. Did it ever work?

[timtebeek]

Thanks a lot @DidierLoiseau ; I've pushed up a change that fixes the tests you've provided. 🙏🏻
Could you let me know if that aligns with what you were thinking?

[DidierLoiseau]

Amazing! Thanks for your reactivity as well!

I should be able to try it out next week. However, as I am using the Maven plugin from the command-line, and it seems I can’t override the version of the rewrite modules used by the plugin without adding it to the pom (I’m using it with -Drewrite.recipeArtifactCoordinates and my own library, which depends on the newer version). Is there a way to do that? Alternatively: would you mind releasing the plugin as well? 🙏

[timtebeek]

I've just kicked off a build (not yet release) of the rewrite-maven-plugin: That should produce a snapshot version that uses the latest from openrewrite/rewrite. Ideally that's enough to try this out before the scheduled release next Wednesday. 🤞🏻