[DNM] Seeking feedback for SemanticVersion implementation
#12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The logic is mostly pulled right from SwiftPM's `Version`'s and TSC's `Version`'s, with some improvements. Now semantic versions compare correctly, by taking into account the pre-release and build metadata identifiers. A lot of test cases are added. 1 initialiser is deprecated. TODO: disallow empty identifiers TODO: disallow leading zeros in numeric identifiers Note on `SemanticVersion`'s `Codable` conformance: empty arrays of pre-release and build metadata identifiers are not encoded, for round-trip equality empty strings in pre-release and build metadata identifiers are encoded, for round-trip equality and consistency with SwiftPM and TSC pre-release and build metadata arrays containing single empty string precede empty arrays in comparison, for consistency with SwiftPM and TSC. SemVer 2.0.0 is ambiguous about this. Can do follow-up commit/pr for this. Not guaranteed that SwiftPM and TSC can accept an alternative comparison rule. proceed to next pair of identifiers if current pair is numerically equal
docc didn't exactly conform `SemanticVersion` to `LosslessStringConvertible`, but it extended it with all the functionalities.
Tests have not been updated from the previous commit, and there are still lots of documentation missing. But because of the renaming of so many files upstream, it's now difficult to rebase the changes here. Going to open a WIP PR as is for feedback, and reapply the changes at the current upstream head.
Comment on lines
+153
to
+180
| extension SymbolGraph.SemanticVersion: Comparable { | ||
| // Although `Comparable` inherits from `Equatable`, it does not provide a new default implementation of `==`, but instead uses `Equatable`'s default synthesised implementation. The compiler-synthesised `==`` is composed of [member-wise comparisons](https://github.com/apple/swift-evolution/blob/main/proposals/0185-synthesize-equatable-hashable.md#implementation-details), which leads to a false `false` when 2 semantic versions differ by only their build metadata identifiers, contradicting SemVer 2.0.0's [comparison rules](https://semver.org/#spec-item-10). | ||
| /// <#Description#> | ||
| /// - Parameters: | ||
| /// - lhs: <#lhs description#> | ||
| /// - rhs: <#rhs description#> | ||
| /// - Returns: <#description#> | ||
| @inlinable | ||
| public static func == (lhs: Self, rhs: Self) -> Bool { | ||
| !(lhs < rhs) && !(lhs > rhs) | ||
| } | ||
|
|
||
| /// <#Description#> | ||
| /// - Parameters: | ||
| /// - lhs: <#lhs description#> | ||
| /// - rhs: <#rhs description#> | ||
| /// - Returns: <#description#> | ||
| public static func < (lhs: Self, rhs: Self) -> Bool { | ||
| let lhsVersionCore = [lhs.major, lhs.minor, lhs.patch] | ||
| let rhsVersionCore = [rhs.major, rhs.minor, rhs.patch] | ||
|
|
||
| guard lhsVersionCore == rhsVersionCore else { | ||
| return lhsVersionCore.lexicographicallyPrecedes(rhsVersionCore) | ||
| } | ||
|
|
||
| return lhs.prerelease < rhs.prerelease // not lexicographically compared | ||
| } | ||
| } |
There was a problem hiding this comment.
I think one of the most important changes is the Comparable conformance. SemVer 2.0.0 specifies that pre-release participates in version comparison, but build metadata doesn't.
Actually right when I'm writing this right now, I just realised that < can be re-implemented like this:
(lhs.major, lhs.minor, lhs.patch, lhs.prerelease) < (rhs.major, rhs.minor, rhs.patch, rhs.prerelease)
Comment on lines
+11
to
+26
| extension SymbolGraph.SemanticVersion { | ||
| /// A storage for pre-release identifiers. | ||
| internal struct Prerelease { | ||
| /// The identifiers. | ||
| internal let identifiers: [Identifier] | ||
| /// A pre-release identifier. | ||
| internal enum Identifier { | ||
| /// A numeric pre-release identifier. | ||
| /// - Parameter identifier: The identifier. | ||
| case numeric(_ identifier: Int) | ||
| /// An alphanumeric pre-release identifier. | ||
| /// - Parameter identifier: The identifier. | ||
| case alphanumeric(_ identifier: String) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Because the precedence between pre-release identifiers depends on whether they're numeric, their numeric-ness is recorded this way right after each identifier is validated during the version's initialization.
3 tasks
|
closing this because it's superseded by #36 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation is largely borrowed from SwiftPM's and TSC's
Version(at least as of the penultimate commit in this PR), but with some changes that make it fully compliant to SemVer 2.0.0. I intend to apply the roughly same changes (the parts that make sense to each) to SwiftPM's, TSC's, and docc's too.It's not fully implemented yet (test cases are outdated (and should be ignored for now) and lots of documentation is missing), but the logic is basically done. Because of the large number of files renamed in the "main" branch, it's difficult to rebase my branch, so even though it's not finished, I'm putting it up here for some feedback.
I estimate that it will probably take another month (but likely sooner) before I get everything ready.