-
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 FlattenStrategy.race
#233
Conversation
|
The context & use site of the strategy & the operator are different enough not to be confusing IMO. |
Could it be called simply |
What does "amb" actually mean? I personally feel it would be better to have a more intention-revealing name, and link back to the equivalent Rx version. |
(Build failure seems unrelated) I also don't know what |
I'm guessing Edit: Nope, it's should for ambiguous. https://msdn.microsoft.com/en-us/library/hh211783(v=vs.103).aspx#Anchor_1 |
Sources/Flatten.swift
Outdated
@@ -31,6 +31,15 @@ public enum FlattenStrategy: Equatable { | |||
/// The resulting producer will complete only when the producer-of-producers | |||
/// and the latest producer has completed. | |||
case latest | |||
|
|||
/// Only the events from the first input producer should be considered for |
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 little ambiguous. Should be "from the first input producer to send an event".
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.
Updated comments in cf6c4eb.
Sources/Flatten.swift
Outdated
@@ -733,6 +754,124 @@ private struct LatestState<Value, Error: Swift.Error> { | |||
var replacingInnerSignal: Bool = false | |||
} | |||
|
|||
extension SignalProtocol where Value: SignalProducerProtocol, Error == Value.Error { | |||
/// Returns a signal that forwards values from the first signal sent on | |||
/// `self`, ignoring values sent on consecutive inner signals. |
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.
Likewise, I don't think this is an accurate description.
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.
Updated comments in cf6c4eb.
Sources/Flatten.swift
Outdated
|
||
private struct FirstState<Value, Error: Swift.Error> { | ||
var outerSignalComplete: Bool = false | ||
var innerSignalComplete: Bool = true |
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.
It seems a little weird to default this to true
. It might be better to default to false
and check for innerSignalComplete || active == nil
in shouldComplete
?
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.
Thanks for this catch! I could remove this redundant flag now in 819550f.
Sources/Flatten.swift
Outdated
case .completed: | ||
let shouldComplete: Bool = state.modify { | ||
$0.outerSignalComplete = true | ||
return $0.active == nil |
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.
What should the behavior be if the outer signal receives a signal, completes, but the inner signal hasn't sent an event?
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 haven't written the test yet, but I think the result should be the same as:
it("should complete when the outer signal completes before sending any signals")
.
https://github.com/ReactiveCocoa/ReactiveSwift/pull/233/files#diff-9cd786f9e52e786771d96cf8c42d3befR1481
New test will be: it("should complete when the outer signal completes before inner signals send any event")
.
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 will also update ambiguous test titles too)
I think I like race. first conflicts with the instance method, but it's also a little ambiguous about whether it's the first signal to be sent or the first signal to send an event. I suppose you could also get confused with race like that—but it seems a little more likely to direct you to the documentation. |
OK, I will rename to |
In 9630f9f, I had to fix To fix that, I had to restore additional state New
|
This is a breaking change because it adds a
|
The PR should target |
@inamiy Would you mind resolving the conflicts here? I think this is ready to be merged to master for a 2.0 release. |
Sorry if I'm late commenting on this, but does this really add value? Isn't this equivalent to: signal
.take(first: 1)
.flatten(anyStrategy) |
No, your snippet takes only one inner signal and flattens it. |
Discussion here last time you asked this. 😉 |
OMG lol, that's embarrassing @mdiep 😓 |
Ship it then :D |
9630f9f
to
76d457e
Compare
Rebase done in 76d457e 😉 (but CI alive?) |
Reopen to trigger CI. |
Sources/Flatten.swift
Outdated
guard isWinningSignal else { return } | ||
|
||
// Dispose all running innerSignals except winning one. | ||
disposableHandle.remove() |
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.
Is it possible to make these not run for every event? They just need to be run once after all.
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 in 1b83292.
Sources/Flatten.swift
Outdated
|
||
innerSignal.observe { [unowned innerSignal] event in | ||
|
||
let isWinningSignal: Bool = state.modify { |
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.
Please name the arguments for multi-line closures.
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 in 1b83292.
Sources/Flatten.swift
Outdated
|
||
switch event { | ||
case .completed: | ||
let shouldComplete: Bool = state.modify { |
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.
Ditto.
Sources/Flatten.swift
Outdated
observer.send(error: error) | ||
|
||
case .completed: | ||
let shouldComplete: Bool = state.modify { |
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.
Ditto.
Implemented #138.
Ref: ReactiveX - Amb operator
I'm still wondering whether
.first
is really a good naming since there's already a blockingfunc first()
.I personally feel
race
is a better name.