Fix comparison issues #11
Conversation
|
Thanks so much for submitting a pull request. We appreciate your contribution! We require all contributors to sign our Contributor License Agreement (CLA) before we can merge any code, and we don't currently have a record of a signed CLA for @chriseplettsonos. Please email [email protected] and let us know you need to sign a CLA, and we'll get the template over to you. |
1 similar comment
|
Thanks so much for submitting a pull request. We appreciate your contribution! We require all contributors to sign our Contributor License Agreement (CLA) before we can merge any code, and we don't currently have a record of a signed CLA for @chriseplettsonos. Please email [email protected] and let us know you need to sign a CLA, and we'll get the template over to you. |
There was a problem hiding this comment.
This is great, thanks for the contribution, @chriseplettsonos !
We'll merge this once the CLA is in place.
Hey Chris. Yes, but there was some information missing. I CCed you on the reply I sent asking for the additional information. I'll drop you another email now. |
|
Fixes #14 |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
As implemented, there were a couple of problems where the library was not performing comparisons between semvers correctly. The first issue is that the presence of buildmetadata was causing the library to treat the version as unstable, which also meant that, given the comparison implementation, the buildmetadata was being considered when comparing two semvers. This is a violation of Section 10 of the semver.org spec, which specifies that “Build metadata MUST be ignored when determining version precedence”. Additionally, it is not specified by semver.org that that the presence of buildmetadata should be involved in determining the stability of a semver. The second problem is that the library uses simple ASCII lexical ordering to compare the pre-release string of two semvers, which does not match the rules defined in Section 11.4 of the semver specification. This change addresses these problems with the following changes 1) Remove the build.isEmpty check from the implementation of the isStable property 2) Implement a PreReleaseIdentifier enum and add Comparable conformance to an array of these types, per the rules laid out in Section 11.4 of the semver specification 3) Add a computed property that returns an array of PreReleaseIndentifiers by parsing the preRelease string 4) Update the less than operator implementation to apply the above changes appropriately 5) Add additional test cases to verify the new behavior Testing: 1) Existing unit tests pass 2) New unit tests pass
chriseplettsonos
force-pushed
the
prerelease_comparisons
branch
from
99f7b55 to
8544443
Compare
|
@finestructure Just curious if this (and my other PR) might be merged soon? If not, we can continue working off of our fork, but I'd love to see these changes released. Thanks! - Chris E. |
|
Apologies @chriseplettsonos , this had completely fallen off my radar. Thanks for the ping! |
|
Just noticed the CodeQL failure Ignoring that. |
As implemented, there were a couple of problems where the library was not performing comparisons between semvers correctly.
The first issue is that the presence of buildmetadata was causing the library to treat the version as unstable, which also meant that, given the comparison implementation, the buildmetadata was being considered when comparing two semvers. This is a violation of Section 10 of the semver.org spec, which specifies that “Build metadata MUST be ignored when determining version precedence”. Additionally, it is not specified by semver.org that that the presence of buildmetadata should be involved in determining the stability of a semver. Only the presence of pre-release should be used to determine the stability of the version.
The second problem is that the library uses simple ASCII lexical ordering to compare the pre-release string of two semvers, which does not match the rules defined in Section 11.4 of the semver specification.
This change addresses these problems with the following changes
Testing: