Enhancements for ExpressionParser

[shayke]

This library is great!
I was missing some range comparisons as described in https://github.com/npm/node-semver
Here is what I've added:

• Support the caret (^) operator
• X-Ranges: Implemented for *, x and X
• Improved Tilde ranges to work with the examples at https://github.com/npm/node-semver#tilde-ranges-123-12-1

I'd really appreciate if this can be merged so I could use this library the same way it behaves in JS (mostly for implementing stuff for Bower and NPM).
The key for this pull request is to be able to use Version.satisfies() and simply provide it with the supported ranges from bower and npm.

[shayke]

Any word on this?

[zafarkhaja]

Sorry, totally forgot about this. Will take a closer look at it tonight.

[zafarkhaja]

Having taken a quick look at the code and having read through the node-semver's ranges spec, I can't help but wonder why would anyone need anything except the x-ranges. Correct me if I'm wrong but it looks like the x-ranges cover all possible use cases for ranges. Besides x-ranges are more intuitive than corresponding tilde ranges and caret ranges, compare ~1.2 vs 1.2.* and ^1.2.3 vs 1.*. Can you provide an example when it's not the case?

P.S. This is not to say I'm not willing to incorporate the changes, just want to understand the wisdom behind these complications.

[shayke]

The tilde and caret provides more fine-grained possibilities to control mostly patches IMO.
Take for example ~1.2.3 which expresses >=1.2.3 <1.3.0.
You can use that for example to skip the patch version of 1.2.2 but keep using up-to-date patches. You cannot do that with just X-Range unless you provide another OR/AND criteria which just looks complicated.

BTW, your example for ^1.2.3 is not accurate, it doesn't mean 1.* but rather every major 1.* which is at least 1.2.3 (e.g: >=1.2.3 <2.0.0)

If you examine few npm and bower packages, you will find that the caret and tilde are used quite often.

[zafarkhaja]

Thank you for taking the time to explain this to me. Now I get the part I was missing.
I looked into your changes and made some comments, please let me know if you need any clarifications.

[kickroot]

Guys, thanks for your work on this! I look forward to the next release that includes this PR...

[zafarkhaja]

@kickroot thanks for the feedback, I appreciate it. Hopefully you can expect the next release this week.

[shayke]

ping @zafarkhaja

[zafarkhaja]

@shayke thank you for your patience and commitment to this PR, I appreciate it.

There were few things that concerned me about this PR

1. Increasing context-sensitivity, such that short forms would mean different things in different contexts:
   • >1 => >1.0.0 and 1-2 => 1.0.0-2.0.0
   • 1 => >=1.0.0 & < 2.0.0
2. Breaking changes in parsing of the short forms where 1 used to mean =1.0.0
3. Something confusing about ExpressionParser.parseVersionExpression() method

Although p.1 complicates the grammar and its parsing, I believe we can at least make it transparent for users just by giving some good examples in the README file. After giving it some thought p.2 started to look like a normal evolution of the grammar, so it's not that scary anymore especially when the new behavior is more common. The real blocker for me was p.3. I didn't realize at first what did bother me but after looking into it I figured out that the source of confusion was this change here which did complicate the ExpressionParser.parseVersionExpression() method. The thing is, it's not EOL (End Of Line) that makes it "version expression" (which we probably should rename to "wildcard expression").

Consider the following expression which doesn't pass the check 2 | 3. 2 here isn't followed by EOL but still it's a wildcard expression and should be interpreted as >=2.0.0 & <3.0.0. That's why we need a different approach to identifying wildcard expressions.

ExpressionParser.parseVersionExpression() method needs some refactoring, but first we need to amend the grammar in its JavaDoc as follows

<version-expr> ::= <wildcard>
                 | <major>
                 | <major> "." <wildcard>
                 | <major> "." <minor>
                 | <major> "." <minor> "." <wildcard>

<wildcard> ::= "*" | "x" | "X"

and then rewrite the method accordingly (this snippet here can serve as a starting point). We also need to amend ExpressionParser.parseExpression() and ExpressionParser.isVersionExpression() methods so that wildcard expressions end up in the right method for parsing. I think we should swap places of isVersionExpression() and isRangeExpression() checks in the ExpressionParser.parseExpression() method because there might be confusion.

Another thing not to forget is to update ExpressionParserTest.shouldParseShortFormOfVersion().

I know this all might be puzzling and confusing and I'm willing to help because it requires substantial refactoring, it's just that I'm not sure how do we go about collaborating in your repo before merging into the main one.

[shayke]

Well then, I guess you can just fork the initial project and take my commits as patches (I could give you that, a patch for each commit?)

The easiest way will probably be to merge this pull and then create another one with all those refactorings you plan to make.

[zafarkhaja]

Well then, I guess you can just fork the initial project and take my commits as patches (I could give you that, a patch for each commit?)

Ideally I'd go with this only if we had "atomic" commits (one per piece of functionality), and that would make more sense then it does now.

The easiest way will probably be to merge this pull and then create another one with all those refactorings you plan to make.

This one is good enough for me, we can do it. But before few more things I'd like to ask you, could you please squash your commits into one (perhaps, using rebase) because basically all subsequent commits are just fixups of the first one. Also I haven't received any contribution so far and I don't have a CONTRIBUTING.md file, but if I did I'd have a section about commit messages :) I try to follow these guidelines or similar.

Thank you.

P.S. I'll release the new version after I do this refactoring following your commit.

[zafarkhaja]

@shayke let me know if you need any help. Or, if you don't mind, I can do the squash myself with the authorship retained.

[shayke]

OK I've squashed the commits and changed the commit message.
Let me know if you need any help from my end!

Thanks

[zafarkhaja]

Thanks @shayke. I merged it into the release-0.9.0 branch. I also took the liberty to amend the commit message, hope you don't mind. Now the ball is on my side, I'll do my part and release the new version by the weekends. Stay tuned!

[shayke]

Thanks a lot, I'm looking forward to the release!

[zafarkhaja]

Unfortunately, the refactoring is taking more time than I've expected.

I've reworked the BNF grammar and I'm almost done with the refactoring but there's one thing I'm not sure about. The following line I overlooked while reviewing the code looks very suspicious to me

assertTrue(v.satisfies("2.0.0")); // >=2.0.0

And I didn't find anything similar to it in the node-semver documentation.
Could you please confirm/clarify your intentions?

[zafarkhaja]

Hey @shayke good news! :) Finally I managed to release it. Thank you for the contribution.

[shayke]

Thank you! I'm happy we got it all together eventually.