AddParentPom recipe

[rcsilva83]

Tests fail with "Recipe was expected to make a change but made no changes."

What's changed?

A new AddParentPom recipe is added.

What's your motivation?

I need to add a parent POM to my projects.

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

Discover why is the recipe making no changes (as stated by the tests) and how to better share code with ChangeParentPom recipe.

Anyone you would like to review specifically?

@timtebeek (as he invited me to make a draft PR on #4239)

Have you considered any alternatives or workarounds?

Any additional context

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]

Great start @rcsilva83 ! Thanks for kicking this off. I've pushed a change to your PR to get the tests running. You had overridden isAcceptable, but for the precondition check to work you'd need to instead override a visit method, and return a changed instance to trigger the guarded visitor. SearchResult.found does exactly that. Hope that helps you explore what the recipe actually should do, as it seemed you got stuck at this point.

[rcsilva83]

Hi @timtebeek !

Thank you very much for your help. The recipe is almost done: it's working but I still need to handle new parent's properties and dependency management.

However, I have some doubts:

1. What to do when no versions are found when using wildcard version? (see line 152)
2. Should I always search for version on Maven repository or should I bypass this when the version is fixed? From what I saw on other recipes this kind of optimization isn't done.

Regards!

[rcsilva83]

I still need to handle new parent's properties and dependency management.

Actually, do you think is there anything to do on this regard? I though I needed to remove properties and dependency management declarations from the pom.xml if they exist on the new parent pom but I don't think that's the case. The ChangeParentPom recipe just add properties and dependency management declarations from the old parent pom, right?

If you can confirm this to me, I'll remove some unused code from the recipe and, after you feedback on the 2 questions I made before, it will be done.

[timtebeek]

Thanks a lot @rcsilva83 ! To answer your questions I'll reply to each in a separate paragraph below.

In ChangeParentPom we seem to skip with no changes when no acceptable version is found. Probably best to be consistent then, even if it could make for a frustrating debugging experience when there are misconfigurations.

I wouldn't skip over validating the parent; we'd like to be sure the parent exists, since we'll use it to update the model of the code, and might otherwise lead to invalid configuration.

As for updating properties: I'd maybe only go as far as doing a doAfterVisit(new org.openrewrite.maven.RemoveRedundantDependencyVersions().getVisitor()); that clears out one of the major reasons to add a parent pom.xml; if there's more clean ups to be done then I suggest to similarly move those to separate recipes, such that there's less complexity in each, with the opportunity for reuse.

Is there anything else you'd then still like to push before a review and merge?

[rcsilva83]

In ChangeParentPom we seem to skip with no changes when no acceptable version is found. Probably best to be consistent then, even if it could make for a frustrating debugging experience when there are misconfigurations.

I'll adjust this and make a new push. Then I think it'll be ready for merge :)

[rcsilva83]

@timtebeek I need your help on a new problem. After changing the change to make no changes when no suitable version is found, I still got an empty <parent/> in the and, because it was added in the visitDocument method. I couldn't find a way to remove it, so I moved all the code to visitDocument method and removed visitTag method.

Everything is fine but apparently the doAfterVisit(new org.openrewrite.maven.RemoveRedundantDependencyVersions().getVisitor()) isn't working anymore and the versions matching with parent aren't being removed anymore.

How can I solve this? I couldn't find any situation like this in other recipes.

[timtebeek]

All tests seem to pass; are we missing anything? Wondering because of your question above and the draft status. Ideally we have a failing test if there's anything to add still.

We're planning a new release tomorrow; let me know if this should make it in in the current form, or with small changes.

[timtebeek]

Some initial feedback already, with a few questions that might lead to stripping out some functionality still.

[rcsilva83]

Hi @timtebeek ! I'll take a look on you feedback and make the code changes needed. But I committed the failing tests I told before so you can understand the problem. I just solve them moving the code to visitDocument method but the RemoveRedundantDependencyVersions stopped working.

[timtebeek]

Nice work here @rcsilva83 ! The reason RemoveRedundantDependencyVersions stopped working is because you had removed the call to maybeUpdateModel(). That ensures our model is aware of the Maven parent before we make subsequent changes.

I've applied a few more polishing commits, and with those I think we're ready to merge. Thanks again!

[rcsilva83]

@timtebeek, and I thank you for the guidance and patience! Now I know why OpenRewrite is improving so fast and have such a vibrant community :)

Regards!

[timtebeek]

Thank you! Couldn't do it without so many folks pitching in. You're changes have already landed in
https://github.com/openrewrite/rewrite/releases/tag/v8.29.0
I'll follow up with downstream releases of recipe modules and the OpenRewrite plugins, culminating in our rewrite-recipe-bom over the next couple hours: https://github.com/openrewrite/rewrite-recipe-bom/releases