Support wider version numbers.

edwardgeorge commented 8 years ago

What does this PR do?

Swaps int to long in NormalVersion. Updates parsing to handle the larger possible numbers.

Added a test to VersionParserTest.java to cover this scenario.

How should this be manually tested?

> VersionParser.parseVersionCore("9007199254740991.9007199254740991.9007199254740991")
==>9007199254740991.9007199254740991.9007199254740991
> new VersionParser('9007199254740991.9007199254740991.9007199254740991').parse(null)
==>9007199254740991.9007199254740991.9007199254740991
> Version.valueOf('9007199254740991.9007199254740991.9007199254740991')
==>9007199254740991.9007199254740991.9007199254740991
> new NormalVersion(9007199254740991, 9007199254740991, 9007199254740991)
==>9007199254740991.9007199254740991.9007199254740991

Any background context you want to provide?

Improves some compatibility with node-semver. Node supports version numbers larger* than can be fitted into an int. The version given in the test that I added (9007199254740991.9007199254740991.9007199254740991) is a real version number found in the wild and was in a data-set I was processing which necessitated this change.

_*node-semver also has an upper limit on version numbers due to max safest number that can be represented exactly by javascript's Number type (Number.MAX_SAFE_INTEGER==9007199254740991), but that arbitrary limit is ignored here. It's probably no coincidence that it is the same value as the number above that I found in the wild. I have also found some version numbers even larger in my dataset from before node and npm imposing a limit_

zafarkhaja commented 8 years ago

Hey @edwardgeorge! While I appreciate the contribution I have mixed feelings about this particular one. On the one hand the total length of such version strings doesn't exceed the recommended 255 characters (see the SemVer's FAQ), but on the other hand it contradicts good judgment, IMHO.

I also understand why someone would want to have a kind of Version.MAX_VALUE but you can still do that using Integer.MAX_VALUE.

Some of my concerns

Why long and not BigInteger?
Why do we need compatibility with the javascript's Number.MAX_SAFE_INTEGER?
What if someone uses the javascript's Number.MAX_VALUE?

I think I'll open an issue at mojombo/semver to ask the community what they think of it. Maybe we'll be able to clarify this on the specification level.

edwardgeorge commented 8 years ago

Why do we need compatibility with the javascript's Number.MAX_SAFE_INTEGER?

this doesn't add compatibility with that at all. it only increases the width to longs to support version numbers greater than can be stored in an int, I just commented at the bottom that the node version has that arbitrary limitation of MAX_SAFE_INTEGER for which I deliberately haven't added compatibility here to jsemver.

As to the motivation, this patch was required in order to complete some batch-data processing of real-world package information (the data set includes the entirety of npm, unfortunately unfiltered by author's good judgement wrt versioning ;) ).

Why long and not BigInteger?

this betrays the fact that i'm not a java developer.

I also understand why someone would want to have a kind of Version.MAX_VALUE but you can still do that using Integer.MAX_VALUE.

I don't really follow this point, i'm afraid. I think I may have confused things a little by mentioning Number.MAX_SAFE_INTEGER which doesn't really have anything to do with these changes. sorry.

zafarkhaja commented 1 year ago

Hello Edward, I hope you're doing well!

Sorry it took me this long to get back to you.

I appreciate you taking the time and effort to contribute, but unfortunately this change is incomplete and doesn't cover all the bases. I wasn't sure if you'd be still interested in completing it, and, anyway, wanted to speed things up to clear the backlog as soon as possible, so I went ahead and implemented the feature myself (017ffe7).

Have a nice day,

zafarkhaja / jsemver