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 scala adapters for doOnEach operator. #537

Merged
merged 5 commits into from
Dec 4, 2013
Merged

Add scala adapters for doOnEach operator. #537

merged 5 commits into from
Dec 4, 2013

Conversation

landonf
Copy link
Contributor

@landonf landonf commented Nov 27, 2013

This re-integrates support for doOnEach that was lost in the Scala refactor.

@cloudbees-pull-request-builder

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

@samuelgruetter
Copy link
Contributor

Thank you @landonf
Imho you should first fix #532
Because then you can get rid of the [U >: T] and the .asInstanceOf[rx.Observable[T] in the Scala code.

@cloudbees-pull-request-builder

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

@landonf
Copy link
Contributor Author

landonf commented Dec 2, 2013

@samuelgruetter I'm happy to take a look; can you expand a bit on what needs doing?

@samuelgruetter
Copy link
Contributor

Ok, first, concerning this PR, I think the only change we need is to replace

def doOnEach[U >: T](observer: Observer[U]): Observable[T]

by

  def doOnEach(observer: Observer[T]): Observable[T]

Now concerning #532 : Apparently it's not strictly necessary to resolve it to make this PR work, because implicit conversions perform even more magic than I would have excpected (or in other words: I'd be interested to see if you could implement this PR without implicit conversions and without #532 , but that's only out of curiosity, there's no purpose in not using implicit conversions).

In #532 , I'd like the following Java code (as well as all similar examples using different overloads of doOnXxx) to work:

    class Fruit {}
    class Apple extends Fruit {}

    Action1<Fruit> printFruit1 = new Action1<Fruit>() {
        public void call(Fruit f) {
            System.out.println(f.getClass().getName());
        }
    };

    public void test1() {
        Observable<Fruit> o = Observable.from(new Fruit(), new Apple());
        o.doOnEach(printFruit1).subscribe();
    }

    public void test2() {
        Observable<Apple> o = Observable.from(new Apple(), new Apple());
        o.doOnEach(printFruit1).subscribe();
    }

With the current Observable.java, test2 will not compile. But if you appropriately replace Action1<T> by Action1<? super T> in Observable.java, then it will, and you will have fixed #532 .

@benjchristensen
Copy link
Member

@samuelgruetter @headinthebox @landonf Should this be merged, or should it be changed as per the last comment?

@samuelgruetter
Copy link
Contributor

Just remove this [U >: T] as I described above. Only reason I didn't do this myself is that I wanted to "introduce" @landonf to the "magic" of Scala generics+variance ;-)

@cloudbees-pull-request-builder

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

@landonf
Copy link
Contributor Author

landonf commented Dec 4, 2013

@samuelgruetter For which I'm much obliged. I've updated this pull request, and submitted a new request for #532.

@samuelgruetter
Copy link
Contributor

Now it looks good.
Btw doOnError and doOnCompleted could be added as well ;-)

benjchristensen added a commit that referenced this pull request Dec 4, 2013
Add scala adapters for doOnEach operator.
@benjchristensen benjchristensen merged commit 66a6b4c into ReactiveX:master Dec 4, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
Add scala adapters for doOnEach operator.
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
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

4 participants