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

Observable.x(ConversionFunc) to allow extensions to Observables #3082

Merged
merged 1 commit into from Aug 7, 2015

Conversation

stealthcode
Copy link

No description provided.

public void testExtend() {
final TestSubscriber<Object> subscriber = new TestSubscriber<Object>();
final Object value = new Object();
Observable.just(value).x(new ConversionFunc<Object,Object>(){
Copy link
Member

Choose a reason for hiding this comment

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

This is essentially the same as:

Observable.just(value).to(o -> {
    o.subscribe(subscriber);
    subscriber.assertNoErrors();
    subscriber.assertCompleted();
    subscriber.assertValue(value);
    return subscriber.getOnNextEvents().get(0);
});

Copy link
Author

Choose a reason for hiding this comment

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

See my other comment regarding exposing the Observable inside the conversion.

@stealthcode
Copy link
Author

The Travis CI failure is related to an rx backpreasure test

rx.BackpressureTests > testMergeAsyncThenObserveOn FAILED
    java.lang.RuntimeException: Unexpected onError events: 1
        at rx.observers.TestSubscriber.assertNoErrors(TestSubscriber.java:263)
        at rx.BackpressureTests.testMergeAsyncThenObserveOn(BackpressureTests.java:138)
        Caused by:
        rx.exceptions.MissingBackpressureException

Is this known to be buggy? My changes seem unrelated. It also failed for #3060.

@akarnokd
Copy link
Member

It didn't fail with MissingBackpressureException as far as I know.

@stealthcode
Copy link
Author

@akarnokd
Copy link
Member

Found the bug in merge(), it was my oversight. Fix posted: #3093.

@stealthcode
Copy link
Author

Awesome! Thanks @akarnokd

@akarnokd
Copy link
Member

I've restarted this build and it passes now.

@stealthcode
Copy link
Author

I've simplified the problem this is trying to address to just adding the x(ConversionFunc) method and unit tests to show how we might use this. My apologies to the general populace who aren't familiar with Battlestar Galactica who may be confused by the references made to said series in my unit test.

@stealthcode
Copy link
Author

I would have liked to have made the static method

private static Subscription Observable.subscribe(Subscriber<? super T>, Observable<T>)

be agnostic of the Observable and instead have the signature..

public static Subscription Observable.subscribe(Subscriber<? super T>, OnSubscribe<T>)

This would allow us to reuse this SafeSubscriber wrapping and the hooks invocations from other Observables (see test) but then the hooks api would break on...

hook.onSubscribeStart(observable, observable.onSubscribe).call(subscriber);

and then it probably wouldn't belong in Observable.java...

@stealthcode
Copy link
Author

Reviewed with @benjchristensen and he agreed to merge this and start testing it out in snapshots.

* @param <R> the return type
*/
@Experimental
public interface ConversionFunc<T, R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need another type? Would Func1<OnSubscribe<T>, R> be okay

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Updating code now.

@stealthcode
Copy link
Author

Latest version was rebased onto 1.x and has ConversionFunc replaced with Func1.

abersnaze added a commit that referenced this pull request Aug 7, 2015
Observable.x(ConversionFunc) to allow extensions to Observables
@abersnaze abersnaze merged commit 97747fb into ReactiveX:1.x Aug 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants