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

Add merge(with:) operator #600

Merged
merged 7 commits into from
Feb 7, 2018
Merged

Conversation

ra1028
Copy link
Contributor

@ra1028 ra1028 commented Feb 6, 2018

Refer #597

Checklist

  • Updated CHANGELOG.md.

/// - returns: A signal that sends all values of `self` and given signal.
public func merge(with other: Signal<Value, Error>) -> Signal<Value, Error> {
return Signal.merge(self, other)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still ought to have a test for this (and the SignalProducer variant below). Would you mind adding one?

/// - parameters:
/// - signals: A sequence of signals to merge.
/// Merges the values of all the given signals, in the manner described
/// by `merge(with:)`.
Copy link
Member

Choose a reason for hiding this comment

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

Normally I wouldn't mind this, but Xcode does not make it easy to find this documentation, so I'd keep it duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I was according to zip, combineLatest operator, though should revert?

Copy link
Member

Choose a reason for hiding this comment

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

We should duplicate it in those too IMO (at least until SourceKit & CMD+Click actually works consistently in Xcode). For now can you keep these how they were?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, will revert them:+1:
Write the doc for zip and combineLatest, will too much lines.
I think there are pros and cons, so not to write now.

@ra1028
Copy link
Contributor Author

ra1028 commented Feb 6, 2018

I added tests

/// - parameters:
/// - signals: A sequence of signals to merge.
/// Merges the values of all the given signals, in the manner described
/// by `merge(with:)`.
Copy link
Member

Choose a reason for hiding this comment

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

We should duplicate it in those too IMO (at least until SourceKit & CMD+Click actually works consistently in Xcode). For now can you keep these how they were?


let mergedSignals = Signal.merge([signal1, signal2])
let mergedSignals = Signal.merge([signal1, signal2, signal3])
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making this more complex now 👍

@mdiep
Copy link
Contributor

mdiep commented Feb 7, 2018

Awesome! Thanks for adding this. 🤘

@mdiep mdiep merged commit 29af9d8 into ReactiveCocoa:master Feb 7, 2018
@ra1028 ra1028 deleted the feature/merge-with branch February 7, 2018 12:57
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

3 participants