Skip to content
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

Merged
merged 1 commit into from
Dec 18, 2016

Conversation

sharplet
Copy link
Contributor

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.

@liscio
Copy link
Contributor

liscio commented Dec 14, 2016

I almost feel like it'd make more sense to have Signal.pipe() just return a new type (like Pipe) that works similarly to a RACSubject stand-in.

It'd just be a struct that holds input and output, or signal and observer, but we could take it further and have it conform to the right protocols so it'd behave like the old RACSubject so we can use operators with it directly. 😛

@sharplet
Copy link
Contributor Author

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?

@andersio
Copy link
Member

Adding labels to tuples is addictive and straightforward. If it gets merged before #137 then it would be in 1.0.

Introducing a new type is however breaking, and I'd prefer to leave that to 2.0, which is just a few months away anyway.

#147

@mdiep
Copy link
Contributor

mdiep commented Dec 15, 2016

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.
@sharplet
Copy link
Contributor Author

I've pushed a fix for the tests.

I thought these names were no longer part of the type

What I found was that if I removed labels from the Pipe typealias, things compiled as expected (i.e., the labelled and unlabelled versions of the tuple were treated as compatible). So the error we were seeing appears to result from assigning a labelled tuple to a differently-labelled tuple.

So this change will break any clients who've used a typealias to add their own labels to this tuple.

@andersio
Copy link
Member

Seems like this would be a 2.0 thing then.

@mdiep
Copy link
Contributor

mdiep commented Dec 16, 2016

I'm okay with merging this as a last-minute fix for 1.0. Thoughts?

@sharplet
Copy link
Contributor Author

My guess is that this will have a low impact in practice, so I'm in board with including it for 1.0.

@mdiep mdiep merged commit 0c8af48 into master Dec 18, 2016
@mdiep mdiep deleted the as-labelled-pipe branch December 18, 2016 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants