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

Fix NPM loose comparisons #36

Merged
merged 4 commits into from Sep 13, 2019
Merged

Fix NPM loose comparisons #36

merged 4 commits into from Sep 13, 2019

Conversation

kmck
Copy link
Contributor

@kmck kmck commented Sep 12, 2019

Hey, thanks for this library!

I noticed that loose requirements weren't evaluating as expected for NPM. It seems like this is because null minor/patch versions default to zero, but I believe the correct behavior is to stop comparison at any missing parts of the semver.

For example, "2.0.2" satisfies "=2" is more like "2 satisfies "=2" than "2.0.2" satisfies "=2.0.0", if that makes sense.

I changed isGreaterThan and isEqualTo when type is SemverType.NPM so that it more closely matches the behavior in Node. I had to fix one erroneous test that asserted that new Semver("2.0.1", SemverType.NPM).satisfies(">2.0 <3.0") should be true (it should ignore the patch version because the requirement isn't fully qualified). All the rest were fine. I added a couple more tests for good measure.


Update:

I tried to add a few more test cases and noticed that they failed. After digging in a little, it looks like the implementation of toReversePolishNotation was a little off.

Everything is supposed to get added to one side of the queue, but apparently some tokens were getting added to the other side instead, perhaps to make unary operators work. I think the trick for unary operators is not to add the two tokens to the front of the queue, but to add them in the back in reverse order.

I flipped req1 and req2 in evaluateReversePolishNotation to keep the tests happy, but the way it evaluates is still equivalent (woo commutative property).

I believe these changes address the unexpected behavior mentioned in #15 and #29

@vdurmont
Copy link
Owner

Looks perfect! Thanks for this super high quality PR! I'll merge and release a new version soon with those changes.

@vdurmont vdurmont merged commit e7a12ac into vdurmont:master Sep 13, 2019
@vdurmont
Copy link
Owner

@kmck v3.1.0 is now available on Maven Central and includes your fix.

@kmck
Copy link
Contributor Author

kmck commented Sep 13, 2019

Cool, thanks for the fast release!

shuyingz pushed a commit to shuyingz/semver4j that referenced this pull request Oct 18, 2019
* .gitignore vs code files
* fix npm loose comparison
* update tests
* fix toReversePolishNotation
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

2 participants