-
Notifications
You must be signed in to change notification settings - Fork 429
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
Add conformance to SignalProducerConvertible for Result #606
Conversation
That's a neat idea! Any chance you can add a few tests for this? |
I didn't because there isn't any current conformance tests at all, but I can. |
Sources/ResultExtensions.swift
Outdated
|
||
extension Result: SignalProducerConvertible { | ||
public var producer: SignalProducer<Value, Error> { | ||
switch self { |
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.
You can use the initializer here:
ReactiveSwift/Sources/SignalProducer.swift
Line 156 in 55ac251
public init(result: Result<Value, Error>) { |
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.
Will do, but perhaps that should now be deprecated in favour of using the protocol?
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.
Done 👍
@NachoSoto Tests added. |
@@ -484,6 +484,34 @@ class SignalProducerSpec: QuickSpec { | |||
expect(error) == operationError | |||
} | |||
} | |||
|
|||
describe("init(_:) base") { |
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 be Result.producer
, right?
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.
Well technically I'm testing SignalProducer.init(_ base:)
as well but I can call it Result.producer
.
error = $0 | ||
} | ||
|
||
expect(error).to(beNil()) |
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.
You should check that the value was sent, and that it completed.
@NachoSoto Amended tests to address comments. |
CHANGELOG.md
Outdated
@@ -1,6 +1,7 @@ | |||
# master | |||
*Please add new entries at the top.* | |||
|
|||
1. New conformance of `Result` to `SignalProducerConvertible` (#606, kudos to @Qata) |
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.
Perhaps this?
Result
now interoperates withSignalProducer
n-ary operators as a constant producer.
@andersio Amended |
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 quite nice. 👍
Checklist
This implements the suggestion in Improve interoperability with
Result
. #409