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

SignalProducer.on: restore value as the last parameter #104

Conversation

dmcrodrigues
Copy link
Contributor

@dmcrodrigues dmcrodrigues commented Nov 22, 2016

Conceptually, value should be defined before failed since that goes in line with the natural order of events, however this reordering can introduce some confusion/inconvenience while using trailing closures specially during the migration from ReactiveCocoa 4.x where value, formerly known as next, was the last parameter and now represents disposed.

This can silently introduce some problems after migration if a project is using .on with a trailing closure to inject side-effects on values without being interested in the value itself.

producer.on { _ in 
    // Inject side-effect, e.g. invalidate context
}

⚠️ Note ⚠️

If we do not go forward with this we need to fix an inconsistency between SignalProducer and Signal where the former was not updated accordingly.

Conceptually, `value` should be defined before `failed` since that goes
in line with the natural order of events, however this reordering can
introduce some confusion/inconvenience while using trailing closures
specially during the migration from ReactiveCocoa 4.x.

There's also an inconsistency between `SignalProducer` and `Signal`
where the former was not updated accordingly.
@dmcrodrigues
Copy link
Contributor Author

@NachoSoto this was discussed on Slack today and I opened a PR to discuss what should we do.

@andersio
Copy link
Member

If we prefer the natural order, adding _: Void = () as the last parameter would prevent the use of trailing closure.

@dmcrodrigues
Copy link
Contributor Author

I thought about that and it's definitely another option but with some quirks.

Closure parameter prior to parameters with default arguments will not be treated as a trailing closure

A warning is raised as expected but can we ignore it?

screen shot 2016-11-23 at 08 48 41

Despite of having a default value, Xcode will ask us a value for our (unnamed) last parameter which I think is far from desirable and can introduce some confusion about the why and what it really means.

Copy link
Contributor

@mdiep mdiep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. Otherwise, I think we need to follow the pattern of startWithFailed, etc.

@andersio andersio merged commit fc36034 into ReactiveCocoa:master Nov 24, 2016
@andersio
Copy link
Member

andersio commented Nov 24, 2016

I don't feel it is frequently used enough to justify a sea of shorthands like how start or observe does. So leaving it as it was in 4.x seems fine.

@dmcrodrigues dmcrodrigues deleted the dr/signalproducer-on-restore-value-as-last-parameter branch November 24, 2016 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants