node-semver icon indicating copy to clipboard operation
node-semver copied to clipboard

[QUESTION] Incorrect loose parsing of version?

Open janslow opened this issue 8 years ago • 11 comments

I'm using the semver.valid() function (with loose = true) to check if strings are possibly semantic versions and I've got an unexpected result with one of the inputs:

> semver.valid("9.4.1208.jre7", false); // Strict, fails as expected
null
> semver.valid("9.4.1208.jre7", true); // Loose, incorrectly parses `patch` number.
'9.4.120-8.jre7'

When in loose mode I'd expect this to either fail to parse it and return null or parse it as 9.4.1208-jre7.

Can anyone explain this behaviour or is it a bug?

janslow avatar Aug 03 '16 15:08 janslow

Also using [email protected]

janslow avatar Aug 03 '16 18:08 janslow

That is indeed quite curious! I agree with your expectation here. This is a bug.

To determine whether this is going to be a breaking change, though, we should check the registry to make sure there aren't any examples of this kind of oddness in the wild.

isaacs avatar Aug 04 '16 04:08 isaacs

I have the expectation that this semver.valid('2.0',false) should return at leas 2.0.0 but is returning null.

blanchma avatar Sep 14 '16 14:09 blanchma

@blanchma, 2.0 isn't a valid semantic version, so must return null in strict mode (i.e., semver.valid(v, false)).

As it still isn't supported in loose mode, IMO, you should create a separate issue about expanding the scope of loose parsing to include MAJOR.MINOR versions, as this issue is about a string which is being parsed into the wrong version.

janslow avatar Sep 14 '16 21:09 janslow

@blanchma Yeah, 2.0 is a version range, which matches any 2.0.x semver. A semver is always 3 parts. Also, that issue is off-topic, so please create a new issue if you'd like to discuss this.

Addressed on https://github.com/npm/node-semver/pull/167

isaacs avatar Sep 14 '16 23:09 isaacs

I have mixed feelings about this, and loose mode in general. Personally I think it makes more sense in extensions/more general version parsers (e.g. https://github.com/megawac/semvish)

megawac avatar Oct 05 '16 17:10 megawac

>>> a = SemVer("2.10.1.1+gdd213db", loose=True)
>>> b = SemVer("2.10.100.1+gdd213db", loose=True)

>>> vars(a)
{'micro_versions': [1], 'minor': 10, 'raw': '2.10.1.1+gdd213db', 'major': 2, 'patch': 1, 'version': '2.10.1.1', 'build': ['gdd213db'], 'loose': True, 'prerelease': []}

>>> vars(b)
{'micro_versions': [], 'minor': 10, 'raw': '2.10.100.1+gdd213db', 'major': 2, 'patch': 10, 'version': '2.10.10-0.1', 'build': ['gdd213db'], 'loose': True, 'prerelease': [0, 1]}

I'm seeing this as well. For some reason, if the patch number is multiple digits, it treats the final number as a prerelease number rather than a micro version.

This is because I'm trying to use commits since tag from git describe --long as a micro_version.

>>> versions = ['2.14.9.0+g98b2e1f','2.14.10.1+geabdbc8','2.14.11.0+gabcdefg']
>>> range_ = '2.X'
>>> print(max_satisfying(versions, range_,loose=True,include_prerelease=True))
2.14.9.0+g98b2e1f
>>> print(max_satisfying(versions, range_,loose=True))
2.14.9.0+g98b2e1f

Even adding include_prerelease=True seems to give unexpected results to max_satisfying.

Mark-Hatch-Bose avatar Apr 21 '19 15:04 Mark-Hatch-Bose

@Mark-Hatch-Bose Yes, that is the same issue as referenced earlier in this issue, which would be addressed by PR #167, but we never landed that, and it would be a breaking change. In general, loose versions are not very well specified, and have some weird edge cases as a result.

Would the solutions discussed in #167 solve your problem?

isaacs avatar Apr 22 '19 23:04 isaacs

From #167

That'd block 1.2.3.4 but 1.2.34.5 would still be misinterpreted as 1.2.3-4.5.

The PR looks hopeful to my understanding but the comment above makes this seem like it's still not quite there yet.

Still confused why a solely numeric version 1.2.34.5 gets converted to 1.2.3-4.5. Is that ever desired? It is understandable with non-numeric values like 1.2.3foo.5 is interpreted as 1.2.3-foo.5 as mentioned in the PR. But with solely numeric values seems to make more sense to require '-' for prerelease interpretation.

Maybe this is best solved with an optional parameter defining the behavior desired?

Or do we know of examples where the above use-case (numeric only loose version desired to be interpreted as prerelease for any single digit beyond patches) is actually being used? It seems farfetched.

Mark-Hatch-Bose avatar Apr 23 '19 11:04 Mark-Hatch-Bose

Still confused why a solely numeric version 1.2.34.5 gets converted to 1.2.3-4.5.

Currently, because loose mode does not require a - to prefix a prerelease section, and 4.5 is a valid prerelease section (and .5 is not), and 1.2.3 is a valid major-minor-patch tuple, it interprets the 1.2.3 as the tuple, and 4.5 as the prerelease.

Is that ever desired?

Not that I can see, no. There aren't a lot of folks using loose mode with >3 numbers in the tuple, as far as I can tell from the inactivity on this issue, which is presumably why it was forgotten for 2 years.

isaacs avatar Apr 23 '19 16:04 isaacs

Ah I see, thanks for the explanation. I didn't understand the parsing order I think.

If the change is made for loose versioning to support 1.2.34.5 (major 1, minor 2, patch 34, micro 5), this would be very helpful for my particular use-case. As mentioned in https://github.com/conan-io/conan/issues/4630, it allows me to convert a git describe --long commits since tag to a micro version, allowing me to uniquely and monotonically increase my versions with every commit. This is used for accurate rebuilding against latest code without requiring SemVer (major, minor, patch) versioning changes for every commit.

Mark-Hatch-Bose avatar Apr 24 '19 13:04 Mark-Hatch-Bose