Skip to content

Add lenient mode to Version string parsing that doesn't require patch version #212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Add lenient mode to Version string parsing that doesn't require patch version #212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions Sources/TSCUtility/Version.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,22 +48,29 @@ public struct Version {
/// An error that occurs during the creation of a version.
public enum VersionError: Error, CustomStringConvertible {
/// The version string contains non-ASCII characters.
/// - Parameter versionString: The version string.
case nonASCIIVersionString(_ versionString: String)
/// The version core contains an invalid number of Identifiers.
case invalidVersionCoreIdentifiersCount(_ identifiers: [String])
/// - Parameters:
/// - identifiers: The version core identifiers in the version string.
/// - usesLenientParsing: A Boolean value indicating whether or not the lenient parsing mode was enabled when this error occurred.
case invalidVersionCoreIdentifiersCount(_ identifiers: [String], usesLenientParsing: Bool)
/// Some or all of the version core identifiers contain non-numerical characters or are empty.
/// - Parameter identifiers: The version core identifiers in the version string.
case nonNumericalOrEmptyVersionCoreIdentifiers(_ identifiers: [String])
/// Some or all of the pre-release identifiers contain characters other than alpha-numerics and hyphens.
/// - Parameter identifiers: The pre-release identifiers in the version string.
case nonAlphaNumerHyphenalPrereleaseIdentifiers(_ identifiers: [String])
/// Some or all of the build metadata identifiers contain characters other than alpha-numerics and hyphens.
/// - Parameter identifiers: The build metadata identifiers in the version string.
case nonAlphaNumerHyphenalBuildMetadataIdentifiers(_ identifiers: [String])

public var description: String {
switch self {
case let .nonASCIIVersionString(versionString):
return "non-ASCII characters in version string '\(versionString)'"
case let .invalidVersionCoreIdentifiersCount(identifiers):
return "\(identifiers.count < 3 ? "fewer" : "more") than 3 identifiers in version core '\(identifiers.joined(separator: "."))'"
case let .invalidVersionCoreIdentifiersCount(identifiers, usesLenientParsing):
return "\(identifiers.count > 3 ? "more than 3" : "fewer than \(usesLenientParsing ? 2 : 3)") identifiers in version core '\(identifiers.joined(separator: "."))'"
case let .nonNumericalOrEmptyVersionCoreIdentifiers(identifiers):
if !identifiers.allSatisfy( { !$0.isEmpty } ) {
return "empty identifiers in version core '\(identifiers.joined(separator: "."))'"
Expand All @@ -85,15 +92,17 @@ public enum VersionError: Error, CustomStringConvertible {
}

extension Version {
// TODO: Rename this function to `init(string: String) throws`, after `init?(string: String)` is removed.
// TODO: Rename this function to `init(string:usesLenientParsing:) throws`, after `init?(string: String)` is removed.
// TODO: Find a better error-checking order.
// Currently, if a version string is "forty-two", this initializer throws an error that says "forty" is only 1 version core identifier, which is not enough.
// But this is misleading the user to consider "forty" as a valid version core identifier.
// We should find a way to check for (or throw) "wrong characters used" errors first, but without overly-complicating the logic.
/// Creates a version from the given string.
/// - Parameter versionString: The string to create the version from.
/// - Parameters:
/// - versionString: The string to create the version from.
/// - usesLenientParsing: A Boolean value indicating whether or not the version string should be parsed leniently. If `true`, then the patch version is assumed to be `0` if it's not provided in the version string; otherwise, the parsing strictly follows the Semantic Versioning 2.0.0 rules. This value defaults to `false`.
/// - Throws: A `VersionError` instance if the `versionString` doesn't follow [SemVer 2.0.0](https://semver.org).
public init(versionString: String) throws {
public init(versionString: String, usesLenientParsing: Bool = false) throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc we needed two constructors instead of one with default value because of downstream dependencies on the exact signatures (passed into map{})

Copy link
Contributor

@WowbaggersLiquidLunch WowbaggersLiquidLunch Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the one that downstream dependencies use is init?(string: String). This initializer still exists (although marked as deprecated) and calls init?(_ versionString: String), which then calls init(versionString: versionString, usesLenientParsing: false) throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the change ready to commit, but also had the thought the downstream dependencies probably use the deprecated function. I could see the use in having a new version ready to cut over to, but that might get into project planning that I don't know the scope of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @colincornaby and @WowbaggersLiquidLunch - lets get this merged first, then @colincornaby you can submit a PR to SwiftPM to use the new methods where appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Thanks!

// SemVer 2.0.0 allows only ASCII alphanumerical characters and "-" in the version string, except for "." and "+" as delimiters. ("-" is used as a delimiter between the version core and pre-release identifiers, but it's allowed within pre-release and metadata identifiers as well.)
// Alphanumerics check will come later, after each identifier is split out (i.e. after the delimiters are removed).
guard versionString.allSatisfy(\.isASCII) else {
Expand All @@ -107,16 +116,16 @@ extension Version {
let versionCore = versionString[..<(prereleaseDelimiterIndex ?? metadataDelimiterIndex ?? versionString.endIndex)]
let versionCoreIdentifiers = versionCore.split(separator: ".", omittingEmptySubsequences: false)

guard versionCoreIdentifiers.count == 3 else {
throw VersionError.invalidVersionCoreIdentifiersCount(versionCoreIdentifiers.map { String($0) })
guard versionCoreIdentifiers.count == 3 || (usesLenientParsing && versionCoreIdentifiers.count == 2) else {
throw VersionError.invalidVersionCoreIdentifiersCount(versionCoreIdentifiers.map { String($0) }, usesLenientParsing: usesLenientParsing)
}

guard
// Major, minor, and patch versions must be ASCII numbers, according to the semantic versioning standard.
// Converting each identifier from a substring to an integer doubles as checking if the identifiers have non-numeric characters.
let major = Int(versionCoreIdentifiers[0]),
let minor = Int(versionCoreIdentifiers[1]),
let patch = Int(versionCoreIdentifiers[2])
let patch = usesLenientParsing && versionCoreIdentifiers.count == 2 ? 0 : Int(versionCoreIdentifiers[2])
else {
throw VersionError.nonNumericalOrEmptyVersionCoreIdentifiers(versionCoreIdentifiers.map { String($0) })
}
Expand Down
Loading