Skip to content

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+11 −16
Draft

chore(model): Remove IVY_VERSION_RANGE_INDICATORS #10441

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fad42a
chore(model): Remove `IVY_VERSION_RANGE_INDICATORS`
sschuberth Jun 5, 2025
0f4854a
docs: Update version range docs to not be limited to Ivy
sschuberth Jun 5, 2025
1527bed
chore(model): Rename a version range function to not be limited to Ivy
sschuberth Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions model/src/main/kotlin/utils/VersionUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ import org.semver4j.RangesList
import org.semver4j.RangesListFactory
import org.semver4j.Semver

/**
* A list of Strings that are used by Ivy-style version ranges.
*/
private val IVY_VERSION_RANGE_INDICATORS = listOf(",", "~", "*", "+", ">", "<", "=", " - ", "^", ".x", "||")

/**
* Return true if the version of this [Identifier] interpreted as an Ivy version matcher is applicable to the
* package with the given [identifier][pkgId].
Expand Down Expand Up @@ -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
Copy link
Member

@fviernau fviernau Jun 5, 2025 •
edited
Loading

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?

Copy link
Member Author

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().

Copy link
Member

@fviernau fviernau Jun 5, 2025 •
edited
Loading

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 the getOrNull().

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?

Copy link
Member Author

@sschuberth sschuberth Jun 5, 2025 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member

@fviernau fviernau Jun 5, 2025 •
edited
Loading

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?

Copy link
Member Author

@sschuberth sschuberth Jun 5, 2025 •
edited
Loading

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.

Copy link
Member

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.


return runCatching {
RangesListFactory.create(version).takeUnless { it.get().isEmpty() }
Expand Down
Loading