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

Updated Transformer's generic parameters #46

Merged
merged 1 commit into from
Sep 29, 2015
Merged

Updated Transformer's generic parameters #46

merged 1 commit into from
Sep 29, 2015

Conversation

ntoskrnl
Copy link

Return type Transformer<T, T> was updated to Transformer<? super T, ? extends T> to support Kotlin type inference.
This should fix #44 issue.

dlew added a commit that referenced this pull request Sep 29, 2015
Updated Transformer's generic parameters
@dlew dlew merged commit 0ed060a into trello-archive:master Sep 29, 2015
@dlew
Copy link
Contributor

dlew commented Sep 29, 2015

Thanks! Also, if you want to submit a PR to upgrade onAttach() I'd take it, too. (I can do it myself, too.)

@ntoskrnl
Copy link
Author

I actually don't know how to correctly change it. Looks like onAttach(Context) appeared in 23 SDK. If I change it for both RxFragment and support.RxFragment, the former one doesn't pass the test. When fragment.onAttach((Context) null) is called from within the test, it fails with NoSuchMethod exception.

@dlew
Copy link
Contributor

dlew commented Sep 29, 2015

Yeah, back-compat can be tricky. I doubt that it's strictly necessary here, but for correctness' sake probably worth it. I'll make an issue.

@almozavr
Copy link

Unhappy with that as compiler can't infer type now, have to write it every time :(

myTypeObservable.compose(RxLifecycle.<MyType>bindView(view));

vs.

myTypeObservable.compose(RxLifecycle.bindView(view));

@dlew
Copy link
Contributor

dlew commented Nov 30, 2015

You could write a wrapper around the current RxLifecycle that uses <T, T>.

@bensandee
Copy link
Contributor

To be honest, the 0.3.1 seems like a step backwards for what I suspect is the predominant platform -- Java 7 + Retrolambda -- because of the fact that it won't infer types properly with these updated signatures. The change may have made Kotlin support cleaner, but it seems to be at the expense of standard Java which is likely to have many more users. But yes, a wrapper is easy enough to write but the same could be said for Kotlin users, no?

@vanniktech
Copy link
Contributor

This change also broke the existing code in our applications.

@bensandee
Copy link
Contributor

Because I'm obsessed to use the newest rev of every library, I'm working on this alternative bind wrapper function in my base activity. Here's what I've come up with -- any comments as to whether this is the best possible?

  @SuppressWarnings("unchecked")
  @Nonnull
  public final <T> Observable.Transformer<T, T> localBindToLifecycle() {
    return (Observable.Transformer<T, T> )this.<T>bindToLifecycle();
  }

@vanniktech
Copy link
Contributor

I reverted back to 0.3.0. Currently I don't have the time to deal with those changes.

@dlew
Copy link
Contributor

dlew commented Nov 30, 2015

Another fix is to use the Java 8 compiler, since it can infer the types correctly.

It's perfectly safe to use Java 8 on Android (just avoid using any newer constructs like lambdas unless you're using something like retrolambda).

@bensandee
Copy link
Contributor

I am using the Java 8 compiler + Retrolambda to rehost onto Android/Java 7 and it's not inferring properly for me. The 0.3.0 works fine, but 0.3.1 forces me to use the this.<T>bindToLifeCycle to compile.

@dlew
Copy link
Contributor

dlew commented Nov 30, 2015

I've made #58 to discuss this further, instead of polluting an old PR.

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.

Type Inference fails in Kotlin
5 participants