-
Notifications
You must be signed in to change notification settings - Fork 994
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
Implement maven version specification #10524
Conversation
fbc8cb6
to
85ff4fa
Compare
28b838e
to
bc53568
Compare
3b7af4d
to
51035f2
Compare
Thanks @amazimbe , as discussed, can you add the test coverage information for before/after? Also, what's the plan to actually start using this version code instead of the old one? |
That will come in my next PR. I had a look at it briefly yesterday and some of it is just updating references from Gem::Version to Dependabot::Maven::Version. I also need to define a |
For the current version.rb For new_version.rb |
8695056
to
8c8cefd
Compare
what about the original code coverage numbers? |
In this PR I'm just adding new_version.rb and that's what has 100% code and branch coverage. I didn't change maven's version.rb in this PR so the code coverage stays at 100% and branch coverage at 88.57%. |
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.
Approving pending resolution of comments and @sachin-sandhu 's review
a02fd5b
to
8feac20
Compare
8feac20
to
9d39e03
Compare
params( | ||
first: T::Array[T.any(String, Integer)], | ||
second: T::Array[T.any(String, Integer)] | ||
).returns(T.nilable(Integer)) |
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.
any reason this is a nilable Integer?
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.
This one is mostly because the call on line 43 is nilable. I tried to change this one but sorbet keeps complaining.
What are you trying to accomplish?
Implement the maven version specification described here: https://maven.apache.org/pom.html#Version_Order_Specification.
Our current maven version implementation is not consistent with the maven specification above. In some cases, this causes versions to be compared incorrectly and inconsistently.
Fix issues related to version precedence or malformed version errors.
Checklist