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 drop(skip) and dropRight(skipLast) to rxscala #1056

Merged
merged 1 commit into from
Apr 21, 2014

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Apr 20, 2014

No description provided.

@cloudbees-pull-request-builder

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

@vigdorchik
Copy link
Contributor

Please no dropRight! This API is so incompatible with reactive model, that I'd relay it to the user to subscribe and aggregate what he wants, rather than promoting this API to Observable. Just my 2c though.

@benjchristensen
Copy link
Member

@vigdorchik What is your reason for this? Rx has had this operator for a very long time (called skipLast).

It is indeed not useful for infinite streams, but there are several operators like that including concat and reduce that are useful when they fit the use case, but useless when it's an infinite stream. Not all operators are meant for all use cases. Even the zip operator can be very problematic if misused (combining slow and fast streams for example).

@vigdorchik
Copy link
Contributor

Ah, ok. My usecase was indeed infinite streams, and I wasn't prepared for finite case.

If I were designing the API now, I would suggest Future instead of Observable for finite case, delegating to map on Lists, but it's again just me :)

@headinthebox
Copy link
Contributor

Future instead of Observable for finite case

That is not as simple as you think. Which future to use? And also, Futures are hot, so this can subtely change the order of side effects. In other words xs.DoSomeOperationThatReturnsASingleElement() should not start processing unless you subscribe to it, or if xs is already hot.

@benjchristensen
Copy link
Member

This discussion comes up every so often about letting the type communicate things like finite vs infinite and scalar vs vector. It was discussed to some degree here: https://groups.google.com/d/topic/rxjava/MBtpYmUJauE/discussion

In Rx.Net they have Task and Observable which work well together. On the JVM there are many different Future implementations, some great, some not. The java.util.concurrent.Future is not usable so we would either have to create yet another Future-like type that works with Observable (can combine them, etc) or choose an implementation and make RxJava dependent on it (not desirable).

Beyond this comes the more nuanced arguments of whether a scalar response (which ultimately encompasses - or could encompass - finite streams that are aggregated down to a single response) should be typed differently. You can read the link above to see more on that and the compositional challenges it causes (types get lost quickly when composing).

@vigdorchik
Copy link
Contributor

@headinthebox I see, Futures don't capture subscription. So it's not Future, but is it a full-blown Observable?
@benjchristensen Yes, we could extract something, make it composable and not call it Future :)
I'll read the discussion to see what complications it causes.
Sorry guys for bringing up the old topic, I should have read the discussions first.

benjchristensen added a commit that referenced this pull request Apr 21, 2014
Add drop(skip) and dropRight(skipLast) to rxscala
@benjchristensen benjchristensen merged commit a2274d3 into ReactiveX:master Apr 21, 2014
@benjchristensen
Copy link
Member

Not a problem for bringing it up ... it's an interesting one and one that doesn't have a clear answer. If you want to discuss it further, either revive the mailing list thread, or start a new Github Issue.

@headinthebox
Copy link
Contributor

+1.

@DavidMGross
Copy link
Collaborator

Is it true that skipLast() will cache "all items" until onCompleted (as the
new addition to the javadoc comments states)?

It seems that it shouldn't have to do such extensive cacheing. For
skipLast(n) it should only need to cache n items; for skipLast(t), only
those items that have arrived during that time window.

On Mon, Apr 21, 2014 at 10:25 AM, headinthebox notifications@github.comwrote:

+1.


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

David M. Gross
PLP Consulting

@akarnokd
Copy link
Member

I've been thinking about the timed skipLast and it could emit old enough elements in onNext and thus freeing up the internal buffer.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 22, 2014

@akarnokd Yes, I think we only need to cache the items in the specified time window.

@zsxwing zsxwing deleted the scala-drop branch April 22, 2014 02:11
@zsxwing
Copy link
Member Author

zsxwing commented Apr 22, 2014

Please help review the new implementation #1065

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

7 participants