Skip to content

Adding support to create self closing Observables from Cursors #76

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

Merged
merged 1 commit into from
Nov 24, 2014

Conversation

dpsm
Copy link
Contributor

@dpsm dpsm commented Nov 23, 2014

An optional overriden method allows a mapper function to create
instances of a type from the Cursor as the Observable iterates
over it.

Signed-off-by: David Marques dpsmarques@gmail.com

@dpsm
Copy link
Contributor Author

dpsm commented Nov 23, 2014

@mttkay @JakeWharton this pull request is for #71

/**
* This class adapts a Cursor into an Iterable.
*/
public class CursorIterableAdapter implements Iterable<Cursor> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value in this abstraction? Why not just write a normal subscribe func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was trying to leverage the Observable.from(Iterable) but looking at it now if I follow your suggestion there will be probably less code overall anyway.

@ronshapiro
Copy link
Contributor

@dpsm have you checked out Venmo's IterableCursor implementation? We used it a lot with Rx. This may be in the realm of "samples/examples/suggestions" since it's an external implementation, but I don't see anything additional here outside of explicitly calling cursor.close() when the stream terminates.

@dpsm
Copy link
Contributor Author

dpsm commented Nov 24, 2014

@ronshapiro yeah I agree we should make people aware of that type of stuff that added to RX makes things easy 👍

I have updated the pull request with the simplest and safest implementation I could think of.

@JakeWharton
Copy link
Contributor

Needs a rebase and squashed. I can take care of it in about 30m if you haven't done it by then.

@dpsm
Copy link
Contributor Author

dpsm commented Nov 24, 2014

@JakeWharton I am on it :)

This commit removes CursorIterableAdapter.java and implements the
Cursor navigation and safe closing in one place.

The option to use doOnTerminate(..) will not work as well to close
the Cursor since downstream operators while emiting errors would
not pass through the operator closing the Cursor.

Signed-off-by: David Marques <dpsmarques@gmail.com>
JakeWharton added a commit that referenced this pull request Nov 24, 2014
Adding support to create self closing Observables from Cursors
@JakeWharton JakeWharton merged commit 48653fc into ReactiveX:0.x Nov 24, 2014
@JakeWharton
Copy link
Contributor

I wonder what the tests for this would have looked like with MatrixCursor. I'm not a huge fan of using mocking like this.

@dpsm
Copy link
Contributor Author

dpsm commented Nov 24, 2014

I can put something together based on matrix cursor with a few test rows
added.
On Nov 24, 2014 6:57 PM, "Jake Wharton" notifications@github.com wrote:

Merged #76 #76.


Reply to this email directly or view it on GitHub
#76 (comment).

@JakeWharton
Copy link
Contributor

It shouldn't matter. There's already a ton of tests which are written in the same way. Besides, we end up relying on Robolectric's implementation rather than a real implementation (although in this case I'm pretty sure it's the real code).

@Avinash-Bhat
Copy link

Any idea when this will be available from the maven central?

Maven Central shows that the version is 0.23.0 which is updated 13-Nov-'14. So rx.android.content.ContentObservable is not available until now (first commit was on 22-Nov).

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.

4 participants