-
Notifications
You must be signed in to change notification settings - Fork 432
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
Move SignalProducerProtocol.then
to the concrete type.
#343
Conversation
91c09af
to
9086ec2
Compare
Sources/SignalProducer.swift
Outdated
return _then(replacement) | ||
} | ||
|
||
internal func _then<U>(_ replacement: SignalProducer<U, Error>) -> SignalProducer<U, 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.
Why have this?
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.
Not having this leads to infinite self recursion in the new overloads, given how Swift selects an overload for a given name.
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.
Can you add a comment that explains that?
@@ -2696,6 +2721,6 @@ extension SignalProducer { | |||
} | |||
} | |||
|
|||
return SignalProducer.attempt(operation) | |||
return SignalProducer(operation) |
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.
Maybe add attempt
as a label? Having so many unnamed initializers leads to ambiguity.
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.
A test case was added specifically for this set of overloads, and it doesn't appear to be ambiguous. Apparently having the return type as Self
helps mitigate the ambiguity that would otherwise arise when you put these two static attempt
in a concrete type extension, like SR-4436.
Moreover, it is expected to be primarily called with trailing closures anyway, and a label can't help in this regard.
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 prefer the static func attempt
name for that reason. It also lends itself to RACformatting.
SignalProducer
.attempt {
…
}
.flatMap(.concat) {
…
}
Is renaming this an essential part of the PR? If not, could we move it off the protocol here and discuss the renaming in a separate PR?
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 is. I will open a separate PR for it then.
This coincidentally dodged the return type ambiguity of
SignalProducer.Type.attempt
, which somehow managed to get through as a protocol extension [but not as a concrete type extension].
Sources/SignalProducer.swift
Outdated
return _then(replacement) | ||
} | ||
|
||
internal func _then<U>(_ replacement: SignalProducer<U, Error>) -> SignalProducer<U, 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.
Can you add a comment that explains that?
/// | ||
/// - returns: A producer that sends events from `self` and then from | ||
/// `replacement` when `self` completes. | ||
public func then(_ replacement: SignalProducer<Value, Error>) -> SignalProducer<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.
This was added to provide a more specific variant for type inference? If so, can you add a //
comment that documents that?
Is this inference covered by a test case?
@@ -2696,6 +2721,6 @@ extension SignalProducer { | |||
} | |||
} | |||
|
|||
return SignalProducer.attempt(operation) | |||
return SignalProducer(operation) |
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 prefer the static func attempt
name for that reason. It also lends itself to RACformatting.
SignalProducer
.attempt {
…
}
.flatMap(.concat) {
…
}
Is renaming this an essential part of the PR? If not, could we move it off the protocol here and discuss the renaming in a separate PR?
9086ec2
to
198e1e6
Compare
SignalProducerProtocol.then
to the concrete type.
198e1e6
to
a446c19
Compare
Addressed all comments. |
a446c19
to
c948353
Compare
Follow-up to #304.
then(_:)
is disambiguated by introducing two more specific variants.