Skip to content
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

Correctly handle prereleases/ANY ranges in subset #377

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Mar 22, 2021

An "ANY" range (ie, "", *, etc.) does not include prerelease
versions except when includePrerelease flag is set.

Also, merely looking at the max/min boundaries of any ranges ignores the
fact that the sub range maybe including prerelease versions that are
excluded from the super range. For example, >=1.2.3-pre.0 is not a
subset of >=1.0.0, because it inludes 1.2.3-pre.0, 1.2.3-pre.1,
and so on.


@jameschensmith, if you would be so kind as to give this a look over, and make sure it fixes the bug you found, I would greatly appreciate it. Thanks!

References

Related to #374, #375 (includes the patch from #375).

Adds short-circuit check if superset is `*`.

PR-URL: #375
Credit: @jameschensmith
Close: #375
Reviewed-by: @isaacs
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cc1ab36 on isaacs/range-subset-with-stars-and-prereleases into 093b40f on master.

isaacs added a commit that referenced this pull request Mar 23, 2021
An "ANY" range (ie, `""`, `*`, etc.) does not include prerelease
versions except when `includePrerelease` flag is set.

Also, merely looking at the max/min boundaries of any ranges ignores the
fact that the sub range maybe including prerelease versions that are
excluded from the super range.  For example, `>=1.2.3-pre.0` is _not_ a
subset of `>=1.0.0`, because it inludes `1.2.3-pre.0`, `1.2.3-pre.1`,
and so on.

PR-URL: #377
Credit: @isaacs
Close: #377
Reviewed-by: @isaacs
@isaacs isaacs force-pushed the isaacs/range-subset-with-stars-and-prereleases branch from cc1ab36 to 57813f7 Compare March 23, 2021 01:36
An "ANY" range (ie, `""`, `*`, etc.) does not include prerelease
versions except when `includePrerelease` flag is set.

Also, merely looking at the max/min boundaries of any ranges ignores the
fact that the sub range maybe including prerelease versions that are
excluded from the super range.  For example, `>=1.2.3-pre.0` is _not_ a
subset of `>=1.0.0`, because it inludes `1.2.3-pre.0`, `1.2.3-pre.1`,
and so on.

PR-URL: #377
Credit: @isaacs
Close: #377
Reviewed-by: @wraithgar
@isaacs isaacs force-pushed the isaacs/range-subset-with-stars-and-prereleases branch from 57813f7 to 0ce87d6 Compare March 23, 2021 01:37
@isaacs isaacs merged commit 0ce87d6 into master Mar 23, 2021
Copy link
Contributor

@jameschensmith jameschensmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, seems I was a bit too late 🙈 I just had a few minor suggestions, but it looks good, though! Still trying to wrap my head around the range comparisons for pre-releases includePrerelease, that's what was taking me a bit 😋 Thanks for addressing this! Much appreciated 😊👍

// - If LT
// - If LT.semver is greater than any < or <= comp in C, return false
// - If LT is <=, and LT.semver does not satisfy every C, return false
// - If any C is a = range, and GT or LT are set, return false
// - If GT.semver has a prerelease, and not in prerelease mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be the following (could be wrong):

Suggested change
// - If GT.semver has a prerelease, and not in prerelease mode
// - If LT.semver has a prerelease, and not in prerelease mode

@@ -15,6 +15,29 @@ const cases = [
['>2 <1', '3', true],
['1 || 2 || 3', '>=1.0.0', true],

// everything is a subset of *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should probably say something new, since this is not always true 😅

@lukekarrys lukekarrys deleted the isaacs/range-subset-with-stars-and-prereleases branch February 24, 2022 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants