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

Add semver.minVersion function. #241

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Add semver.minVersion function. #241

merged 1 commit into from
Mar 26, 2019

Conversation

coreyfarrell
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jun 15, 2018

Coverage Status

Coverage increased (+0.05%) to 98.873% when pulling b99ae3b on coreyfarrell:min-version into 6086e5a on npm:master.

@coreyfarrell
Copy link
Contributor Author

I've rebased / resolved the conflicts.

semver.js Outdated Show resolved Hide resolved
@isaacs
Copy link
Contributor

isaacs commented Jan 18, 2019

I think there's an edge case missed here, which isn't likely to come up in practice, but is worth covering for correctness.

If a version has a prerelease on it, and is a < or <= comparator, then that exact tuple won't match, and in the case of <, neither will the exact tuple with the prerelease. And if the range tuple is 0.0.0, then the minver won't match either!

Consider the range <0.0.0-beta. 0.0.0 is not going to match, but 0.0.0-0 will.

I think this can be handled by having another minver of 0.0.0-0, and testing that as well.

Another interesting case is something like >1.2.3-alpha. The minimum version isn't 1.2.3, but... what is it?

(If you had <0.0.0-beta >0.0.0-alpha, then it'd figure out that 0.0.0-alpha.0 is the min, I see.)

Also, it'd be good to have 100% coverage of new code, if possible, especially if it won't be exercised in npm itself. I haven't tested this directly.

@coreyfarrell
Copy link
Contributor Author

I think there's an edge case missed here, which isn't likely to come up in practice, but is worth covering for correctness.

If a version has a prerelease on it, and is a < or <= comparator, then that exact tuple won't match, and in the case of <, neither will the exact tuple with the prerelease. And if the range tuple is 0.0.0, then the minver won't match either!

Consider the range <0.0.0-beta. 0.0.0 is not going to match, but 0.0.0-0 will.

I think this can be handled by having another minver of 0.0.0-0, and testing that as well.

Good catch. I've added this.

Another interesting case is something like >1.2.3-alpha. The minimum version isn't 1.2.3, but... what is it?

(If you had <0.0.0-beta >0.0.0-alpha, then it'd figure out that 0.0.0-alpha.0 is the min, I see.)

I've added this specific test case and with the components in reverse (less than first and greater than first).

Also, it'd be good to have 100% coverage of new code, if possible, especially if it won't be exercised in npm itself. I haven't tested this directly.

The only added code which is not covered is the default case which is unreachable. For now I've added the hint for istanbul to ignore it, I could remove the default case if you prefer.

@isaacs
Copy link
Contributor

isaacs commented Jan 19, 2019

The only added code which is not covered is the default case which is unreachable. For now I've added the hint for istanbul to ignore it, I could remove the default case if you prefer.

If it's truly unreachable, then yeah, just go ahead and remove it, or make it an ignored throw so at least we'll find out some day if it turns out that assumption is incorrect.

@isaacs
Copy link
Contributor

isaacs commented Jan 19, 2019

Another interesting case is something like >1.2.3-alpha. The minimum version isn't 1.2.3, but... what is it?

OIC, I'd missed that, yeah, it just appends a .0 onto those ones. Disregard that comment :)

@coreyfarrell
Copy link
Contributor Author

The only added code which is not covered is the default case which is unreachable. For now I've added the hint for istanbul to ignore it, I could remove the default case if you prefer.

If it's truly unreachable, then yeah, just go ahead and remove it, or make it an ignored throw so at least we'll find out some day if it turns out that assumption is incorrect.

I already added the hint for istanbul to ignore the throw, knowing if a new operator is added in the future seemed like the right way to handle this. That way if someone uses minVersion against a range with a new operator it'll throw instead of giving an incorrect result.

@isaacs
Copy link
Contributor

isaacs commented Jan 19, 2019

Ah, yeah, a new operator in the future could result in that case being hit. Good call 👍

@coreyfarrell
Copy link
Contributor Author

I understand time is short for all but can I do anything to help this move forward?

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.

None yet

4 participants