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

Allow pre-releases when the range uses them #184

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

carolynvs
Copy link

@carolynvs carolynvs commented Aug 9, 2022

Currently, when a range uses a pre-release, such as 1.0.0-alpha.1 - 1.1.0, all pre-release versions are being rejected when the constraint is checked because the upper part of the range does not include a pre-release. For example, 1.0.0-alpha.2 should match that range, but it is rejected right now.

I have updated how we evaluate constraints so that it checks whether a constraint part uses pre-releases OR the larger constraint range that it is part of uses a pre-release. When either part of a range uses a pre-release, the entire range should allow pre-release versions.

Fixes #177

Currently, when a range uses a pre-release, such as 1.0.0-alpha.1 -
1.1.0, all pre-release versions are being rejected when the constraint
is checked because the upper part of the range does not include a pre-release.
For example, 1.0.0-alpha.2 should match that range, but it is rejected
right now.

I have updated how we evaluate constraints so that it checks whether a
constraint part uses pre-releases OR the larger constraint range that it
is part of uses a pre-release. When either part of a range uses a
pre-release, the entire range should allow pre-release versions.

Fixes Masterminds#177

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs carolynvs force-pushed the fix-constraint-range-with-prerelease branch from 68dfa87 to 6b3ebcf Compare August 9, 2022 20:15
@carolynvs carolynvs marked this pull request as ready for review August 9, 2022 20:24
Copy link
Member

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

What happens if someone does a range like 1.0.0-alpha.1 - 1.0.0 <= 1.0.0? That last part isn't part of the range. This is 1.0.0-alpha.1 - 1.0.0 and <= 1.0.0. Is this user error for specifying a second part that doesn't handle a prerelease or should it handle a prerelease?

If we break it down based on each rule, the <= 1.0.0 should not handle a prerelease as it's not part of the rule that includes a prerelease. Reading, I see the rule I wrote should match nothing.

With this PR, passing in 1.0.0-beta.2 would pass as the <= 1.0.0 would allow prereleases.

I found this when I took the PR and added more test cases to it.

Thoughts?

@mattfarina
Copy link
Member

Note, I have ideas how to modify this work so it only checks prereleases on the prerelease ranges. It involves some deeper cutting changes. If you want, I can pick up the work from here and try to change it.

@carolynvs
Copy link
Author

Ope, sorry for the slow turnaround, I was on vacation for a couple weeks there. I'm back now and have a bit of a start on handling the case you added.

@carolynvs
Copy link
Author

After looking at it more, I'm not sure how best to go about only applying the pre-release to a single range, since the range is immediately removed and converted into another representation when the constraint is parsed. If you have an idea for how to make this work well, please have at it! 👍

@draithat
Copy link

Any updates on this request?

Previously, I was detecting if any range in a larger constraint set contained a prerelease, and allowed prereleases for any constraint in the set. This incorrectly allowed prereleases on constraints that did not participate in a range that had prereleases.

When rewriteRange is called, we lose key information about if a constraint was originally part of a range:

1.0.0-alpha.1 - 1.0.0 becomes >=1.0.0-alpha.1, <=1.0.0

I have updated rewriteRange so that it also returns any constraints that were originally part of a range that used a prerelease. Then when we are parsing constraints, allowPrerelease on that constraint is enabled if it was originally detected as participating in a range with prereleases. This ensures that only the individual ranges have allowPrerelease enabled.

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs
Copy link
Author

@mattfarina Sorry for the really long delay getting a fix for the test case you brought up. Please take another look?

@carolynvs
Copy link
Author

I've fixed the merge conflict and this is ready to review.

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.

Allow pre-releases when only used for one part of a range?
3 participants