-
Notifications
You must be signed in to change notification settings - Fork 2
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
move VersionMappings stuff to a separate module and make it public #13
Conversation
Generate changelog in
|
@@ -17,7 +17,7 @@ | |||
|
|||
import org.apache.maven.artifact.versioning.ComparableVersion; | |||
|
|||
final class VersionMapping { | |||
public final class VersionMapping { |
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 suspect we'll need to make the getters public as well
@@ -17,7 +17,7 @@ | |||
|
|||
import org.apache.maven.artifact.versioning.ComparableVersion; |
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 think this is an implementation
dependency, so it can't be exposed with a public getter. Can we abstract it away somehow such that we're not bound to the maven ComparableVersion type?
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.
so one alternative to this is that we could leave VersionMapping
package-private, and instead change the public VersionMappings#getReplacement
to return an Optional<MavenCoordinate>
instead of Optional<String>
. From the perspective of something outside of this module, a caller doesn't care so much with inspecting the individual parts of VersionMapping
and likely only cares about asking "what is the replacement that will be used for this dependency?"
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.
Sounds good to me, reducing the exposed surface is always a plus
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.
cool, fixed
This reverts commit 3b5568a.
…-package-alignment into blaub/split-packages
ComparableVersion getMaxJakartaVersionWithJavaxNamespace() { | ||
return maxJakartaVersionWithJavaxNamespace; | ||
} |
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.
Thoughts on removing this method entirely in favor of building ComparableVersion
on an as-needed basis in VerssionMappings?
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.
yeah we can do that
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.
one downside is that it becomes less clear that this mapping could technically apply to a range of versions, but i think in practice there's only one Jakarta EE release with the old javax
namespace, so it's really always a single version.
public static Map<String, VersionMapping> getMappings() { | ||
return mappings; | ||
} |
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 this should return Collection<VersionMapping>
since the key is an implementation detail for quick lookups? we could return mappings.values();
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.
👍 done
also make VersionMappings#getMappings return Collection<VersionMapping>
Released 0.2.0 |
No description provided.