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

Added support for custom functions in combineLatest. #763

Merged
merged 1 commit into from
Feb 11, 2014

Conversation

chrisgrimm
Copy link

No description provided.

@cloudbees-pull-request-builder

RxJava-pull-requests #676 SUCCESS
This pull request looks good

@vigdorchik
Copy link
Contributor

I think the same can be achieved by calling map over standard combineLatest, no need for a special method.

@vigdorchik
Copy link
Contributor

To illustrate, that's how I do it in my akka with rx monitoring project:
https://github.com/vigdorchik/rxmon/blob/master/src/main/scala/org/matmexrhino/rxmon/Operations.scala#L31

@headinthebox
Copy link
Contributor

I agree with @chrisgrimm that this one should be there.

This one is not very handy (and less performant) since in 99% of the case you will immediately destruct the tuple by applying a function.

@benjchristensen can you merge this?

def combineLatest[U](that: Observable[U]): Observable[(T, U)]

@vigdorchik
Copy link
Contributor

@headinthebox @chrisgrimm yes, the proposed one is more general and doesn't lose performance in 99% of the cases. Maybe the existing one should be removed then?

@headinthebox
Copy link
Contributor

I guess it is the idiomatic Scala way, like Zip, so I think we should keep it just for consistency.

@chrisgrimm
Copy link
Author

My thinking was in line with @headinthebox . The other issue I'd like to see tackled with with the combineLatest function is the ability to zip many Observables together. The current implementation can only zip two things together which makes it hairy to handle larger zippings. You get this kind of "recursive Tuple2" thing going on if you repeatedly zip things, which can be resolved in Scala (http://stackoverflow.com/questions/21154639/higher-order-operations-with-flattened-tuples-in-scala), but it is annoying nonetheless.

@headinthebox
Copy link
Contributor

You may want to look at the join patterns.

On Thu, Jan 23, 2014 at 11:30 PM, Chris Grimm notifications@github.comwrote:

My thinking was in line with @headintheboxhttps://github.com/headinthebox. The other issue I'd like to see tackled with with the combineLatest
function is the ability to zip many Observables together. The current
implementation can only zip two things together which makes it hairy to
handle larger zippings. You get this kind of "recursive Tuple2" thing going
on if you repeatedly zip things, which can be resolved in Scala (
http://stackoverflow.com/questions/21154639/higher-order-operations-with-flattened-tuples-in-scala),
but it is annoying nonetheless.


Reply to this email directly or view it on GitHubhttps://github.com//pull/763#issuecomment-33176602
.

@vigdorchik
Copy link
Contributor

@headinthebox @chrisgrimm right, I see you want to keep the API consistent, though not minimal. To do so however, the current implementation of combineLatest should also be changed to delegate to the new one like it's done in zip.

@headinthebox
Copy link
Contributor

Correct. Send a pull request :-)

On Fri, Jan 24, 2014 at 12:54 PM, Eugene Vigdorchik <
notifications@github.com> wrote:

@headinthebox https://github.com/headinthebox @chrisgrimmhttps://github.com/chrisgrimmright, I see you want to keep the API consistent, though not minimal. To do
so however, the current implementation of combineLatest should also be
changed to delegate to the new one.


Reply to this email directly or view it on GitHubhttps://github.com//pull/763#issuecomment-33217020
.

@vigdorchik
Copy link
Contributor

@headinthebox sure, if @chrisgrimm doesn't beat me to it :)

@vigdorchik
Copy link
Contributor

@chrisgrimm Unfortunately I don't see your fork in the list of forks of Netflix/RxJava, so I can't send a PR to it.

benjchristensen added a commit that referenced this pull request Feb 11, 2014
Added support for custom functions in combineLatest.
@benjchristensen benjchristensen merged commit 4f793bf into ReactiveX:master Feb 11, 2014
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

5 participants