-
Notifications
You must be signed in to change notification settings - Fork 227
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
Reduce the responsibility of ResultProtocol
.
#235
Conversation
@@ -1,14 +1,14 @@ | |||
// Copyright (c) 2015 Rob Rix. All rights reserved. | |||
|
|||
/// An enum representing either a failure with an explanatory error, or a success with a result value. | |||
public enum Result<T, Error: Swift.Error>: ResultProtocol, CustomStringConvertible, CustomDebugStringConvertible { | |||
case success(T) | |||
public enum Result<Value, Error: Swift.Error>: ResultProtocol, CustomStringConvertible, CustomDebugStringConvertible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a separate, breaking change. We should probably do this separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're all for this change, so I'm okay with just doing it here.
Result/ResultProtocol.swift
Outdated
@@ -1,34 +1,14 @@ | |||
// Copyright (c) 2015 Rob Rix. All rights reserved. | |||
|
|||
/// A type that can represent either failure with an error or success with a result value. | |||
// A protocol that can be used to constrain associated types as `Result`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have 3 /
s so it shows up in Xcode's documentation viewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd definitely love to have @ikesyo review this. He has a much better understanding of the consequences of this than I do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm for @mdiep's comments, but otherwise LGTM 👀
🤘 |
Similar to signal and producer operators in ReactiveSwift, method and computed properties are now moved to the concrete type.
ResultProtocol
now requires only aResultProtocol.result
requirement which materializesself
.Caveat: Automatic migration is impossible, but manual migration is not hard (copy & pasting
.result
).