-
Notifications
You must be signed in to change notification settings - Fork 347
chore(model): Remove IVY_VERSION_RANGE_INDICATORS
#10441
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
base: main
Are you sure you want to change the base?
Conversation
d685912
to
406baeb
Compare
These are by now only used for an early return from `getVersionRange()`. As checking for a valid range is done by the `RangesListFactory`, the only remaining case to check for is the empty string, which would otherwise create a ">= 0.0.0" range. Note that as of 8142cdc the variable name was misleading anyway, as it does not only contain Ivy version range indicators. Signed-off-by: Sebastian Schuberth <[email protected]>
406baeb
to
1fad42a
Compare
Should this paragraph be changed too: ort/website/docs/configuration/package-configurations.md Lines 17 to 19 in 0fdf659
|
@@ -66,7 +61,7 @@ internal fun Identifier.isVersionRange(): Boolean { | |||
} | |||
|
|||
private fun Identifier.getVersionRanges(): RangesList? { | |||
if (IVY_VERSION_RANGE_INDICATORS.none { version.contains(it, ignoreCase = true) }) return null | |||
if (version.isEmpty()) return null |
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 function now more often return non-null, compared before.
In such cases, the version is not interpreted just as plain version string anymore, but as version range.
This may also imply functional changes to Identifier.isVersionRange()
which may impact the invariant check in package configuration's constructor. From reading the commit message, it sounds as if this change is not risky. To me it's not clear why this is so.
(Do you plan to make a release for #10434, and if so, can we wait with this PR until after this release is created?)
Note: Before, version range semantics were limited to string containing any of IVY_VERSION_RANGE_INDICATORS
while now this isn't anymore. Do we now support additional ways of
specifying version ranges?
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 function now more often return non-null, compared before.
How do you know? It's just that fewer cases are caught by the early return; but all other cases should still return null
from the getOrNull()
.
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.
How do you know? It's just that fewer cases are caught by the early return; but all other cases should still return
null
from thegetOrNull()
.
True, I don't - but I'm actually looking for an explanation whether we now support additional ways of specifying version ranges. Do you know?
Do you consider this change to be non-function, so basically just dropping some potential performance optimization?
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'm actually looking for an explanation whether we now support additional ways of specifying version ranges. Do you know?
I'm not sure what you mean. Ever since 8142cdc we have implicitly supported not only Ivy, but also NPM and CocoaPods version ranges. It just was never documented.
Do you consider this change to be non-function, so basically just dropping some potential performance optimization?
Yes.
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 you plan to make a release for #10434, and if so, can we wait with this PR until after this release is created?
I could do a patch-release if requested, but the merge if this PR does not have to wait necessarily, as releases can also be done for tags for past commits.
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 you consider this change to be non-function, so basically just dropping some potential performance optimization?
Yes.
Can you share the explanation why the change is non-functional? This would also resolve
I'm not sure what you mean. Ever since ...
...by now we only supported version ranges which have at least one of the version range indiciator chars. So, you are saying that Semver
lib does not have any additional string representations of ranges?
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.
Can you share the explanation why the change is non-functional?
I simply have no indication why it should not be non-functional.
So, you are saying that Semver does not have any additional representations?
Yes. I mean, that was the point of @MarcelBochtler's change in 883e56c: The indicators were designed after the version ranges that the Semver library supports.
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, can we make this more clear in the commit message that this change is intended to be non-functional along with a rationale why?
Note that as of 8142cdc the variable name was misleading anyway, as it
does not only contain Ivy version range indicators.
This part of the commit message is a bit misleading, because the IVY_
prefix only has been recently added, IIRC to align with its KDoc.
Signed-off-by: Sebastian Schuberth <[email protected]>
Signed-off-by: Sebastian Schuberth <[email protected]>
becd0c4
to
1527bed
Compare
Done in a separate commit. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10441 +/- ##
============================================
- Coverage 56.74% 56.72% -0.02%
Complexity 1640 1640
============================================
Files 337 337
Lines 12476 12475 -1
Branches 1177 1177
============================================
- Hits 7079 7076 -3
- Misses 4945 4947 +2
Partials 452 452
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
These are by now only used for an early return from
getVersionRange()
. As checking for a valid range is done by theRangesListFactory
, the only remaining case to check for is the empty string, which would otherwise create a ">= 0.0.0" range.Note that as of 8142cdc the variable name was misleading anyway, as it does not only contain Ivy version range indicators.