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

Compose generics #1762

Merged
merged 1 commit into from
Oct 18, 2014
Merged

Compose generics #1762

merged 1 commit into from
Oct 18, 2014

Conversation

vadims
Copy link
Contributor

@vadims vadims commented Oct 16, 2014

No description provided.

@benjchristensen
Copy link
Member

This seems to make my examples all work while still passing the covariance unit tests we have (and with easier to read generics).

@davidmoten @akarnokd @zsxwing can you please review this as well?

The Java 8 code I tried is at #1663 (comment) and that code does not compile with current code.

@@ -177,14 +177,14 @@ public void call(Subscriber<? super R> o) {
@SuppressWarnings("unchecked")
public <R> Observable<R> compose(Transformer<? super T, ? extends R> transformer) {
// Casting to Observable<R> is type-safe because we know Observable is covariant.
return (Observable<R>) transformer.call(this);
return (Observable<R>) ((Transformer<T, ? extends R>) transformer).call(this);
Copy link
Member

Choose a reason for hiding this comment

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

Since using casting, why not just use the following code:

return ((Transformer<T, R>) transformer).call(this);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't compile since Transformer is a Func1 that returns a Observable<? extends R>

Copy link
Member

Choose a reason for hiding this comment

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

Right. Sorry.

Is Transformer<T, R> extends Func1<Observable<T>, Observable<R>> better? Is there any reason that using Observable<? extends R>.

@zsxwing
Copy link
Member

zsxwing commented Oct 17, 2014

LGTM except my comment.

@benjchristensen benjchristensen merged commit 357effa into ReactiveX:1.x Oct 18, 2014
@benjchristensen
Copy link
Member

Manually merged in #1776

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.

3 participants