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

Fixing the generics for merge and lift #860

Merged
merged 1 commit into from
Feb 12, 2014

Conversation

abersnaze
Copy link
Contributor

I've tracked down to two problems #858

The first is that lift wasn't quite right. I changed the lift argument from Operator<R, T> to Operator<? extends R, ? super T>. Unfortunately Java won't let us hide that away in the Operator class so we have to redeclare that everywhere in the debug hooks but it's still better than the alternative of having to use the full Func1<? extends Subscriber<? super R>, ? super Subscriber<? super T>> everywhere.

The second problem was in the return type of merge Observable<T>. Because of operators like merge(Observable<? extends T> t1, Observable<? extends T> t2) the only thing that can be said for the return type is that it is also Observable<? extends T> but at the moment it is returning Observable<T>.

This pull request changes all of them and leads to some silly return values like

Observable<String> a, b;
Observable<? extends String> x = merge(a, b);

@cloudbees-pull-request-builder

RxJava-pull-requests #784 FAILURE
Looks like there's a problem with this pull request

@akarnokd
Copy link
Member

We should avoid return types such as A<? extends B>.

@cloudbees-pull-request-builder

RxJava-pull-requests #787 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

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

@abersnaze
Copy link
Contributor Author

@akarnokd you were right the return type change wasn't necessary. I've reverted and rebased the branch to clean out all the mistakes.

@cloudbees-pull-request-builder

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

@akarnokd
Copy link
Member

I almost did a PR on this when I saw you now did the same thing as I. But was it necessary to change the debugs as well?

@abersnaze
Copy link
Contributor Author

Yes. There is a plugin hook inside lift that the debug plugin implements and spreads from there.

benjchristensen added a commit that referenced this pull request Feb 12, 2014
Fixing the generics for merge and lift
@benjchristensen benjchristensen merged commit b0d975c into ReactiveX:master Feb 12, 2014
@abersnaze abersnaze deleted the merge-generics branch February 13, 2014 07:02
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