Skip to content
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.

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

Fully conform to Semantic Versioning 2.0.0 spec #64

Merged
merged 1 commit into from Dec 16, 2015
Merged

Fully conform to Semantic Versioning 2.0.0 spec #64

merged 1 commit into from Dec 16, 2015

Conversation

jakeheis
Copy link
Contributor

This pull request makes the Version class conform fully to the semver spec, fixing https://bugs.swift.org/browse/SR-113.

I was unclear about the function of Specifier in this project, so I did not update that class. I would be happy to do so if need be!

@mxcl
Copy link
Contributor

mxcl commented Dec 11, 2015

Specifier doesn't want support there since it is intended to represent part of the 3 number tuple. Correct me if wrong @mattt

@mxcl
Copy link
Contributor

mxcl commented Dec 11, 2015

Will pull when we have tests. If you haven't the time, I can write them.

@mxcl
Copy link
Contributor

mxcl commented Dec 11, 2015

Great work!

continue
}

let castedLhsIdentifier: Any = Int(lhsPrereleaseIdentifier) ?? lhsPrereleaseIdentifier

Choose a reason for hiding this comment

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

Simple past tense is "cast". So castLhsIdentifier would be correct in a grammatical sense. (In case anyone actually cares )
Then again, I'm not a native speaker... ;)

@mattt
Copy link
Collaborator

mattt commented Dec 11, 2015

@mxcl Correct. The Specifier type is defined to be a sparse counterpart to a Version, where trailing components can be omitted. That said, adding SemVer extensions components to Version with 4a7621 should probably have a corresponding change to Specifier.

@mattt
Copy link
Collaborator

mattt commented Dec 11, 2015

@jakeheis Added some line comments with suggested refactoring. Great work, and thanks for your contribution!

@zeke
Copy link

zeke commented Dec 11, 2015

@mattt! You're alive! :)

@jakeheis
Copy link
Contributor Author

@mattt Thanks for the code review! I've refactored those parts you found confusing, hopefully should be good to go now

@mxcl I'd be happy to take a swing at writing the tests, I'll see what I can do

XCTAssertLessThanOrEqual(v7, v7)
XCTAssertGreaterThanOrEqual(v7, v7)
XCTAssertLessThanOrEqual(v8, v8)
XCTAssertGreaterThanOrEqual(v8, v8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if these tests are supplanted by the others, there is no reason to remove them. It’s possible the tests you have written to supplant these have bugs.

Can we get these back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sure, I didn't think they were necessary anymore but I can add them back in

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I'm sure there aren't bugs with your stuff, but generally with tests I feel more is better and keeping them makes me feel safer.

@mxcl
Copy link
Contributor

mxcl commented Dec 15, 2015

Will merge, if you can squash the history. Thanks.

mxcl added a commit that referenced this pull request Dec 16, 2015
Fully conform to Semantic Versioning 2.0.0 spec
@mxcl mxcl merged commit e143f0c into apple:master Dec 16, 2015
@mxcl
Copy link
Contributor

mxcl commented Dec 16, 2015

👍🏻🎆

ankitspd pushed a commit to ankitspd/swift-package-manager that referenced this pull request Jan 11, 2019
Support command skipping with shouldCommandStart.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants