Implemented: ForEach #131

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants

dcapwell commented Feb 8, 2013

implement #45.

Owner

benjchristensen commented Feb 8, 2013

Hi @dcapwell, thanks for contributing!

The code looks great, only thing I see missing is support for dynamic languages which requires "Object" overloads of any method with Func* or Action* arguments so closures from Groovy/Clojure/Scala/JRuby/etc can be passed in.

For example:

-> typed: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L2212
-> non-typed: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L2269

You'll see how it takes the 'Object' and derives a FuncN from it and then invokes the appropriate typed method:

public static <T> Observable<List<T>> toSortedList(Observable<T> sequence, final Object sortFunction) {
        @SuppressWarnings("rawtypes")
        final FuncN _f = Functions.from(sortFunction);
        return create(OperationToObservableSortedList.toSortedList(sequence, new Func2<T, T, Integer>() {

            @Override
            public Integer call(T t1, T t2) {
                return (Integer) _f.call(t1, t2);
            }

        }));
    }

If you can confirm that the methods work from at least one of the dynamic languages by adding a unit test for each of the overloads I would appreciate it.

Groovy is the primary one we use thus far (most similar to Java): https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-groovy/src/test/groovy/rx/lang/groovy/ObservableTests.groovy

Thank you again!

dcapwell added some commits Feb 8, 2013

dcapwell commented Feb 9, 2013

Is there a better way to handle the errors than just skipping calling the user code?

Owner

benjchristensen commented Feb 9, 2013

Can you point me to a part of the code where that question refers to?

If an error occurs (either in the operator itself or the Func passed in) the appropriate thing (and only option really) is to call onError.

dcapwell commented Feb 9, 2013

https://github.com/dcapwell/RxJava/commit/30f855b021a8feb673a34cda2c5022c8f74e9541#L1R115

If onNext throws an exception, then onError is called and all calls to onNext, onError, or onComplete get ignored after.

Owner

benjchristensen commented Feb 9, 2013

Yes that's the correct behavior to call onError if an exception is thrown.

If the code is working as designed (I think it is!) then it will also automatically unsubscribe to the parent Observable.

You can see how here: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/util/AtomicObserver.java#L60

The Observer is wrapped when the Observable is subscribed to. You can see that here: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L132

That ensures the onError/onCompleted only get called once and onNext will be ignored and takes care of automatically unsubscribing in the event it doesn't get done correctly elsewhere.

dcapwell commented Feb 9, 2013

Would it be fine to make forEach not support static operators? If so then i can have it unsubscribe. Basically just replicate this code:

AtomicObservableSubscription subscription = new AtomicObservableSubscription();
return subscription.wrap(onSubscribe.call(new AtomicObserver<T>(subscription, observer)));

and have onComplete and onError unsubscribe like AtomicObserver does.

@benjchristensen benjchristensen referenced this pull request Feb 14, 2013

Merged

Operator: forEach #147

Owner

benjchristensen commented Feb 14, 2013

Since forEach is basically just a blocking version of subscribe, I'd like to propose a different approach that uses subscribe but blocks until onError or onCompleted are received.

There is very little new code for this (mostly just overloads) and this blocks if the Observable is asynchronous.

Pull Request: Netflix#147

The interesting part of the code is here: https://github.com/Netflix/RxJava/pull/147/files#L1R368

It uses a CountDownLatch to block until onError or onCompleted are received.

Owner

benjchristensen commented Feb 14, 2013

Re-reading the specs ... we should actually eliminate the onError/onCompleted overloads as they are not required.

It should instead just throw an exception if onError is called (since this is blocking) and onCompleted is not needed because it blocks.

http://msdn.microsoft.com/en-us/library/hh211815(v=vs.103).aspx

Owner

benjchristensen commented Feb 14, 2013

See pull request Netflix#147 for more on this.

Owner

benjchristensen commented Feb 15, 2013

Thank you @dcapwell for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment