-
Notifications
You must be signed in to change notification settings - Fork 431
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
Proposal: Label the output and input ends of Signal.pipe() #153
Conversation
I almost feel like it'd make more sense to have It'd just be a struct that holds |
In general I think I agree with that direction. This particular change has the advantage of being non-breaking—however, if we'd like to go down the road of adding a real type for this concept, it may make sense to break it for 1.0. Thoughts, @ReactiveCocoa/reactiveswift? |
The tests need to be updated. Which is surprising to me because I thought these names were no longer part of the type. 😕 This makes sense to me. 👍 But I'd like to see it with passing tests to verify that my assumptions about how it works are correct. |
Attempts to clarify usage by being more explicit about the input and output ends of the signal "pipe", and rewords the documentation accordingly. See also: https://swift.org/documentation/api-design-guidelines/#label-closure-parameters.
4654e4c
to
631b058
Compare
I've pushed a fix for the tests.
What I found was that if I removed labels from the So this change will break any clients who've used a typealias to add their own labels to this tuple. |
Seems like this would be a 2.0 thing then. |
I'm okay with merging this as a last-minute fix for 1.0. Thoughts? |
My guess is that this will have a low impact in practice, so I'm in board with including it for 1.0. |
This is in part inspired by the API Design Guidelines: https://swift.org/documentation/api-design-guidelines/#label-closure-parameters.
Attempts to clarify usage by being more explicit about the input and output ends of the signal "pipe", and rewords the documentation accordingly.