Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ParentPomInsight recursive #4529

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

DidierLoiseau
Copy link
Contributor

@DidierLoiseau DidierLoiseau commented Sep 26, 2024

What's changed?

Made ParentPomInsight recursive, with a parameter to disable recursivity.

What's your motivation?

We needed to check the parents more deeply before performing some cleanup. @sambsnyd suggested updating this recipe instead of creating a new one. @timtebeek suggested to make it the default.

This could also be used to conditionally perform some upgrades, e.g. only perform Spring Boot 3 migration if we are not already in Spring Boot 3 (see also openrewrite/rewrite-spring#361) – although you would also need to check the imported poms in this case.

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

Note how the change affects matchNonSnapshot() which must explicitly disable recursivity to keep the same behavior (otherwise all child modules would match as well).

I also took the opportunity to document Semver.validate(), although the documentation is mostly copied from existing recipes and may need some clarification.

The recipe does not support a versionPattern – I’m not sure how to handle it when version isn’t provided, which could be useful for example if someone wanted to check if any ancestry is a snapshot or release candidate.

Have you considered any alternatives or workarounds?

The current ParentPomInsight only allows checking the direct parent, so you would have to list all possible values instead.

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 – yes, except I reverted the change of indentation the version field multi-line description.

@sambsnyd
Copy link
Member

sambsnyd commented Oct 9, 2024

This sounds a lot like the existing ParentPomInsight recipe. Can that cover this use-case?

@DidierLoiseau
Copy link
Contributor Author

@sambsnyd this one is recursive, but maybe I could make ParentPomInsight optionally recursive as well instead?

(also using SearchResult.found seems simpler than what I did here)

@timtebeek timtebeek marked this pull request as draft October 9, 2024 10:30
@timtebeek
Copy link
Contributor

Still catch up from last week; was great to meet briefly at Devoxx! Indeed seems best to make the existing ParentPomInsight recursive as well; judging by how that's used we should be fine to just change that by default, or at least until someone comes along with a convincing case to be selective.

Feel free to use this same pull request; we'll squash merge anyway, and I imagine it's mostly moving over your test case and introducing a while loop. Thanks again!

@DidierLoiseau DidierLoiseau changed the title HasMavenAncestry recipe Make ParentPomInsight recursive Oct 19, 2024
@DidierLoiseau
Copy link
Contributor Author

@timtebeek & @sambsnyd, I have updated the PR and its description accordingly. I made it recursive by default as suggested.

@timtebeek timtebeek marked this pull request as ready for review October 21, 2024 18:08
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved already, but I before we merge I think we should break that while loop when there are downloading errors ; would you agree there?

Great to see how thorough your are with your tests when adapting an existing recipe!

@DidierLoiseau
Copy link
Contributor Author

Great to see how thorough your are with your tests when adapting an existing recipe!

I try to follow a TDD approach, I find it much more enjoyable than writing tests after, and I also find parameterized tests are a very nice way to check many different cases while staying fairly easy to implement and understand.

@timtebeek timtebeek merged commit 5c07699 into openrewrite:main Oct 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Requested Recipe
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants