-
Notifications
You must be signed in to change notification settings - Fork 371
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
Adding latest and release fields to MavenMetadata, using them in AddDependency #5082
base: main
Are you sure you want to change the base?
Adding latest and release fields to MavenMetadata, using them in AddDependency #5082
Conversation
rewrite-maven/src/test/java/org/openrewrite/maven/AddDependencyVisitorTest.java
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Nullable String latest, | ||
@Nullable String release, |
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.
Would it not make more sense to add these at the end of the argument list, such that we can switch back to (implied) AllArgsConstructor later?
rewrite-maven/src/test/java/org/openrewrite/maven/AddDependencyVisitorTest.java
Outdated
Show resolved
Hide resolved
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.
Do we need this test class at all? Seems to be covered by MavenMetadataTest already, and then I'd rather not maintain the constructor call here if we can avoid it.
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.
I tend to agree, (small) integration tests over unit tests (unless the unit has very nasty code you really need to test to be sure it works correctly).
@Nullable | ||
String latest; | ||
|
||
public Versioning( |
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.
Perhaps good to already deprecate this one, to nudge folks to move over.
public Versioning( | |
@Deprecated | |
public Versioning( |
Co-authored-by: Tim te Beek <tim@moderne.io>
} | ||
|
||
// VisibleForTesting | ||
static String findVersionToUse(@Nullable VersionComparator versionComparator, String defaultVersion, MavenMetadataSupplier mavenMetadataSupplier, |
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.
To be honest, I don't like this architectural change (I also discussed this with @Laurens-W). I think it is better for both component isolation and your mental model to keep helper methods in its own class. So in this case the findVersionToUse
should stay in the InsertDependencyInOrder
visitor.
What's changed?
Two things:
MavenMetadata
"record" - latest and releaseAddDependency
recipe, so that it honors input set tolatest.release
as taking the value from the top-levelrelease
field.What's your motivation?
Address feedback from one of the OpenRewrite users.
Anything in particular you'd like reviewers to focus on?
Yes!
MavenMetadata
changes are handled provide compatibility?AddDependency
) as I read this is the intent in the project. All recipes seem to have their own version matching logic.