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

Reconsider ErrorTypeConvertible #147

Merged
merged 2 commits into from Apr 5, 2016
Merged

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Mar 28, 2016

This was changed in #141. But the current form does not looks correct since the ConvertibleType associated type is not used in any method signature.

This change brings back the original definition removes ConvertibleType from ErrorTypeConvertible and worked well for both Swift 2.2 on OS X and swift-DEVELOPMENT-SNAPSHOT-2016-03-24-a on Linux.

This was changed in #141. But the current form does not looks correct since the `ConvertibleType` associated type is not used in any method signature.

This change brings back the original definition and worked well for both Swift 2.2 on OS X and swift-DEVELOPMENT-SNAPSHOT-2016-03-24-a on Linux.
}

public extension ResultType where Error: ErrorTypeConvertible {
public extension ResultType where Error: ErrorTypeConvertible, Error.ConvertibleType == Error {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added the constraint to make the implementation here safer.

@ikesyo
Copy link
Member Author

ikesyo commented Mar 28, 2016

This is breaking API changes, but Result 2.0 is already released. So should this be shipped as 3.0?

@norio-nomura
Copy link
Contributor

I did not use pre-release identifier of semver on 2.0.0 because of SwiftPM's current behavior.
But, I should mark 2.0.0 as Pre-release on https://github.com/antitypical/Result/releases/tag/2.0.0.
May I change that now?

@ikesyo
Copy link
Member Author

ikesyo commented Mar 28, 2016

Just setting pre-release checkbox on at the GitHub release would not have any effects for SwiftPM or Carthage or CocoaPods, I think.

@norio-nomura
Copy link
Contributor

If we would mark 2.0.0 as pre-release, should we continue marking new releases as pre-release until the release of Swift 3?
I think we may make breaking changes until the release of Swift 3. If we would not mark them as pre-release, we may need to incrementing major version on them.

@ikesyo
Copy link
Member Author

ikesyo commented Mar 28, 2016

But unfortunately, 2.0.0 release is already shipped as non-pre-release version, because the release has no pre-release identifiers as you said. So just adding pre-release mark on GitHub would not help anything.

@norio-nomura
Copy link
Contributor

I'm so sorry that I did not tag 2.0.0 as pre-release.

@norio-nomura
Copy link
Contributor

Is associatedtype ConvertibleType still needed?

@norio-nomura
Copy link
Contributor

Is associatedtype ConvertibleType still needed?

It seems that is still needed.

@norio-nomura
Copy link
Contributor

I have an interest in the codes that would be affected by this change.

@ikesyo
Copy link
Member Author

ikesyo commented Mar 30, 2016

I revisited this, and came to the thought that removing ConvertibleType and public func force<T>() -> T. I believe the latter should not be exposed as a public API and the leakage should be a bug. Considering that, this change is just a bug fix and can be shipped as 2.0.1.

@antitypical/result Any thoughts?

@robrix
Copy link
Contributor

robrix commented Mar 30, 2016

@ikesyo: Agreed 💯% re: force.

@ikesyo
Copy link
Member Author

ikesyo commented Apr 5, 2016

Can I merge this?

@robrix
Copy link
Contributor

robrix commented Apr 5, 2016

Looks good to me! 👍

@robrix robrix merged commit b37ce36 into master Apr 5, 2016
@robrix robrix deleted the reconsider-ErrorTypeConvertible branch April 5, 2016 17:03
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

3 participants