-
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
Drop the label of SignalProducer
initializers that take a sequence.
#120
Conversation
814cf12
to
024b9e1
Compare
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 don't see why this would be better, it always adds clarity in call sites.
I'd argue that the variable names usually have conveyed that they are signals, e.g. |
Most of the time I use this initialiser to compose a signal with other producers, or to use producer-specific operators on it. This means that it's usually part of some kind of operator chain, where a mess of keywords, operators and other syntax can make things dense and hard to parse. I personally find that the |
But, it would be good to show some real examples of how this changes code at the point of use. |
TBH I haven't used it in my projects with all the stuff we have introduced since 4.2.0. The last time I've ever used it is probably in our codebase. As you might have seen in the diff, the label doesn't feel like adding clarity at all but verbosity. Though
|
I'm pretty ambivalent about this. It's nice on the one hand—I always prefer unnamed arguments when they're unambiguous—but this initializer doesn't seem widely used, so it might be a little confusing. |
024b9e1
to
c9c640b
Compare
SignalProducer(signal:)
to SignalProducer(_:)
.SignalProducer
initializers that take a sequence.
In a short discussion with @sharplet, we came up with a more generalised idea of dropping the label for all initializers that take a sequence. The argument was:
Summing up with @sharplet's words:
So the PR has been updated with two Not sure if we should count the vararg initializer as one though. |
I'd recommend avoiding that ambiguity if we can. |
@@ -469,7 +469,7 @@ public final class Property<Value>: PropertyProtocol { | |||
/// - initialValue: Starting value for the property. | |||
/// - signal: A signal that will send values to the property. | |||
public convenience init(initial: Value, then signal: Signal<Value, NoError>) { | |||
self.init(unsafeProducer: SignalProducer(signal: signal).prefix(value: initial)) | |||
self.init(unsafeProducer: SignalProducer(signal).prefix(value: initial)) |
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 do you think about renaming the local variable to values
from signal
? I like the direction of naming the signal to describe its values, rather than just restating that it's a signal (which we know from the types).
@@ -29,7 +29,7 @@ public struct SignalProducer<Value, Error: Swift.Error> { | |||
/// | |||
/// - parameters: | |||
/// - signal: A signal to observe after starting the producer. | |||
public init<S: SignalProtocol>(signal: S) where S.Value == Value, S.Error == Error { | |||
public init<S: SignalProtocol>(_ signal: S) where S.Value == Value, S.Error == 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.
Likewise, what do you think about naming this values
internally?
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'd better stay signal
, since it has more than just values but also termination events.
@@ -100,9 +100,9 @@ public struct SignalProducer<Value, Error: Swift.Error> { | |||
/// - parameters: | |||
/// - values: A sequence of values that a `Signal` will send as separate | |||
/// `value` events and then complete. | |||
public init<S: Sequence>(values: S) where S.Iterator.Element == Value { | |||
public init<S: Sequence>(_ sequence: S) where S.Iterator.Element == Value { |
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.
And this also could remain values
.
@@ -122,7 +122,7 @@ public struct SignalProducer<Value, Error: Swift.Error> { | |||
/// - second: Second value for the `Signal` to send. | |||
/// - tail: Rest of the values to be sent by the `Signal`. | |||
public init(values first: Value, _ second: Value, _ tail: Value...) { |
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 makes sense to me to keep the values:
label here.
I'd like to add that the discussion was informed specifically by this section of the API design guidelines. |
34481be
to
593ae80
Compare
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 a fan of this change 👍
f2d288e
to
c000500
Compare
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 makes some sense to me under the type-conversion section of the Swift API guidelines that @sharplet linked to.
What do you think, @NachoSoto?
@@ -100,7 +100,7 @@ public struct SignalProducer<Value, Error: Swift.Error> { | |||
/// - parameters: | |||
/// - values: A sequence of values that a `Signal` will send as separate |
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 still not a fan of any of this because users are left to the expense of the type inference to determine which initializer to use, rather than being able to explicitly pick one.
If Swift's type inference and diagnostics were great I might not mind, but considering how poor they are, it's my opinion that this will bring more problems than benefit (in fact I can't think of any benefit other than conciseness, which is not necessarily an advantage).
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 doesn't feel like the type checker could ever cause any chaos here though.
SignalProducer
is a discrete time sequence, while Sequence
is immediate. This means we would likely never conform SignalProducer
to Sequence
, or in other words the two signatures would never collide in any sense.
I wish we could have unanimity here, but we do have consensus and—more importantly—the Swift API guidelines seem pretty clear on this point. So I'm going to go ahead and merge. |
Since this is not a lossy conversion, and there is no conflicting signature, the
signal
label does not give any extra clarity at call site.