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

A variant of combinePrevious that doesn't need an initial value. #445

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

andersio
Copy link
Member

@andersio andersio commented Jun 11, 2017

Note that combinePrevious() is not implemented using default arguments and optionals, so that existing use cases as concerned by @NachoSoto would not break.

In other words, combinePrevious(nil) and combinePrevious() are not equivalent, given the constraint <U> Value == Optional<U>.

Checklist

  • Updated CHANGELOG.md.

Copy link
Member

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

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

👎🏻

This could be backwards incompatible if somebody is passing nil because Value is optional.
Furthermore I don't think changing this API is necessary (I'm generally against parameters that fundamentally change the behavior of a function) since this can be replicated with skip(first: 1)

@andersio
Copy link
Member Author

andersio commented Jun 11, 2017

🤔

Haven't considered optionals. That said we can use this just for combinePrevious(), and keeping the original combinePrevious(initial:) so that combinePrevious(nil) would work as is.

since this can be replicated with skip(first: 1)

Not quite if you consider how many extra hops needed for U to U? to (U?, U?) to (U, U) (probably less with a filterMap, but still).

@andersio andersio changed the title Make the initial value of combinePrevious optional. A variant of combinePrevious that doesn't need an initial value. Jun 11, 2017
@sharplet
Copy link
Contributor

For reference, some previous discussion on this general idea: ReactiveCocoa/ReactiveCocoa#2632

@andersio
Copy link
Member Author

andersio commented Jun 12, 2017

@sharplet Thanks for pointing out the previous discussion. My responses to @NachoSoto's arguments:

One of the combinePrevious implementation is not implemented in terms of the other.

Both share the same implementation in this PR.

This operator is redundant: the existing combinePrevious is more general. You can accomplish the same thing by doing something like producer.combinePrevious(nil).skip(1).

From Signal<U, E> to precisely Signal<(U, U), E> with the current combinePrevious, one has to jump through multiple hops:

values // Signal<U, E>
    .optionalize() // Signal<U?, E>
    .combinePrevious(nil) // Signal<(U?, U?), E>
    .filterMap { $0.0 != nil && $0.1 != nil ? ($0.0!, $0.1!) : nil } // Signal<(U, U), E>

which is hardly sane. Moreover, as shown in this PR, combinePrevious() with an optional initial value is actually the most general version, as it backs the original combinePrevious(_:) in a generic context by exploiting the fact that optionality of Value is opaque.

Copy link
Member

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

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

The new implementation is great 👍

@andersio andersio merged commit 98cf568 into master Jun 13, 2017
@andersio andersio deleted the combine-previous branch June 13, 2017 17:37
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