-
Notifications
You must be signed in to change notification settings - Fork 354
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
Added property version migration #4330
Added property version migration #4330
Conversation
…upIdAndArtifactId and ChangeManagedDependencyGroupIdAndArtifactId
…DependencyGroupIdAndArtifactId
…into feature/ChangePropertyVersions
Hi @marcel-gepardec ; Appreciate the effort you've put in here. I do wonder if this would be better as a separate recipe though, as to not complicate the options and logic further for these already not trivial recipes. There's a lot to factor in, such as parents, BOMs and their order, managed dependencies and properties. To add optional renaming on top of that adds complexity which isn't often used, especially considering that these recipes are a often include in framework migrations where the option is not set. That way folks would still see properties not adhering to this specification. Comparatively I'd imagine a separate recipe that only renames properties from whatever they are named currently to a prefix/suffix naming scheme might be easier to maintain going forward. And even then we'd need to take into account if a property is actually overriding a property as the often used Spring managed dependency version properties, and handle that appropriately. What are your thoughts on the above? |
My thoughts on this: There are two issues here:
I also think those two should be separated in different recipes. The first seems to me as expected behavior. It may lead to problems if two or more dependencies use the same property, but after migration the version numbers differ for some reason. What should the recipe do? Override the first change, how high is the risk of breaking things? Change the version tag instead, how complicated is this to implement? |
Thanks for clarifying! Indeed best to separate those two concerns. I'd have expected 1. to already be handled with the recipe we have, also based on the tests & changes I've seen up to now. If this isn't the case then a separate unit test to replicate the issue is probably best, and we can then work from there. Let's split off the 2. concern into a separate issue, if we think it's feasible to implement at all in a reliable manner without supervision. Best not to make those changes part of this PR, however well intended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions could not be made:
- rewrite-java-test/src/test/java/org/openrewrite/java/UseStaticImportTest.java
- lines 336-336
Closing as discussed on #4366 (comment) |
Property version migration
What's changed?
Depending on the content inside the version tag the migration decides if the version is changed inside the dependency tag or inside the properties. I also added a new option to the ChangeDependencyGroupIdAndArtifactId recipe implementation.
The new option is changePropertyVersionNames. It is set per default on false. If it is set on true than it changes the property variable name from whatsoever to the new [artifactId].version
Example
Recipe with changePropertyVersionNames on true:
Before:
After:
What's your motivation?
While migrating one of my projects I noticed that in my pom every single version inside the dependencies changed from a property variable to a normal version.