Add an includePrerelease option for Range.IsSatisfied and related methods

[adamreeve]

Fixes #49

This turned out to be slightly more complicated than I initially thought as it changes the meaning of something like ~1.2.0. Previously that could be treated the same as >=1.2.0 <1.3.0 but that would allow eg. 1.3.0-alpha.1 to satisfy the range when includePrerelease is true, which is not the desired behaviour. To avoid this I've added new operators, GreaterThanOrEqualIncludingPrereleases and LessThanExcludingPrereleases.

This is a breaking change in terms of binary compatibility due to then new optional arguments, and also changes behaviour as previously equal range specifications might no longer be equal.

[KevBelisle]

Why would 1.3.0-alpha satisfy a range of ~1.2.0 ?

It seems like the fix should to (if include prelease is true) make a range of ~1.2.0 be equivalent to >=1.2.0-prerelease && <1.3.0-prerelease, not adding in new operators.

[adamreeve]

Why would 1.3.0-alpha satisfy a range of ~1.2.0 ?

Because as originally implemented, ~1.2.0 desugars to >=1.2.0 < 1.3.0, and 1.3.0-alpha is greater than 1.2.0 and less than 1.3.0

It seems like the fix should to (if include prelease is true) make a range of ~1.2.0 be equivalent to >=1.2.0-prerelease && <1.3.0-prerelease, not adding in new operators.

I considered this, ~1.2.0 could be equivalent to >=1.2.0 < 1.3.0-0, but something like ^0.2.0 causes problems as this is equivalent to >=0.2.0-0 <0.3.0-0 when including pre-releases but >=0.2.0 <0.3.0 when not (it could be argued that this should behave differently but I'm basing this on compatibility with node-semver). Desugaring to >=0.2.0-0 < 0.3.0-0 would allow 0.2.0-1 for example to be valid even when includePrerelease was false which would be incorrect, so adding the extra operators seemed like the best approach.

These operators are an internal implementation detail so users don't need to be concerned directly with this, although the includePrerelease behaviour is less intuitive than if ranges could desugar to normal operators.

Another option could be to delay the desugaring of ranges until IsSatisfied is called and have them desugar to something different depending on the flag value. Re-reading your comment, possibly that's what you meant, but I don't think this provides extra benefit given the operators are internal, and this would be a more complicated change. The includePrerelease argument could also be provided at range construction time instead of on the call to IsSatisfied, but that would seem a bit awkward to me. I had a look at node-semver and this is how they handle this, eg. see https://github.com/npm/node-semver/blob/e08d9167937e09e8e6fe23aacaf17f892a1d69e1/classes/range.js#L294, the caret desugaring behaviour depends on includePrerelease.

[adamreeve]

After thinking about this some more, I think it probably does make more sense for includePrerelease to be an argument on the range constructor rather than the IsSatisfied method. In which case changing the desugaring behaviour rather than introducing new operators would be the way to go.

[sfwester]

After thinking about this some more, I think it probably does make more sense for includePrerelease to be an argument on the range constructor rather than the IsSatisfied method. In which case changing the desugaring behaviour rather than introducing new operators would be the way to go.

FWIW, having includePrerelease as an argument to IsSatisfied would be a nicer api for our use case. Considering the range checking behavior at construction time rather than "check" time would be far away from where we want to consider it. In my mind, a Range is a Range and the includePrerelease is a modification of the IsSatisfied behaviour. I expect we would end up writing some kind of wrapper like the below since our range is constructed way before we have decided what kind of checking we want to do.

bool IsSatisfied(Range range, bool includePrereleases, string versionString)
{
    return new Range(range.ToString(), includePrereleases).IsSatisfied(versionString);
}

However, take this feedback with a grain of salt as I have not dug into this library very much and we can for sure make the Range ctor argument approach work.

[KevBelisle]

@adamreeve, re-reading your previous comment has me doubting something... You said (emphasis mine):

Desugaring to >=0.2.0-0 < 0.3.0-0 would allow 0.2.0-1 for example to be valid even when includePrerelease was false which would be incorrect

If includePrerelease is false, wouldn't ANY pre-release version fail to satisfy the range? If my range is ~1.2.0, and includePrerelease is false, I would NOT expect a version of 1.2.4-alpha to satisfy it.

So, unless I'm mistaken - and I haven't taken a close look at how this is implemented internally - couldn't range desugaring always take into account prerelease versions, knowing that if includePrerelease is false, that will be checked before the versions themselves are even considered?

[adamreeve]

FWIW, having includePrerelease as an argument to IsSatisfied would be a nicer api for our use case

Thanks @sfwester, that matches my original thinking.

If includePrerelease is false, wouldn't ANY pre-release version fail to satisfy the range?

includePrerelease=false wouldn't mean prereleases can never satisfy a range but would match the current behaviour where you can opt in to pre-releases of a specific version by explicitly referencing a prerelease version. Eg. at the moment you can use a range like ~1.2.0-alpha.1, which is satisfied by 1.2.0-alpha.2 for example but not 1.2.1-alpha.2.

The problem with desugaring to a comparator with an added prerelease is that we then can't distinguish between a prerelease added during desugaring and one explicitly added in the range string to opt in to prereleases of a specific version. This isn't really a problem with the < operator but is with >=.

[adamreeve]

I'm going to merge this as is and make a 2.0.0 pre-release, I think on balance this is going to be the simplest approach and given I've got evidence that includePrerelease is more useful on IsSatisfied and no real evidence that it's more useful on Range construction it makes sense to go with that approach.