Fix StyxObservable type parameters #219
Conversation
| return new StyxCoreObservable<>(delegate.map(transformation::apply)); | ||
| } | ||
|
|
||
| public <U> StyxObservable<U> flatMap(Function<T, StyxObservable<U>> transformation) { | ||
| public <U> StyxObservable<U> flatMap(Function<? super T, ? extends StyxObservable<? extends U>> transformation) { |
There was a problem hiding this comment.
I am skeptical about ? extends StyxObservable<? extends U> bit. Due to some technical limitations, we don't intend StyxObservable to be extendable (although it is implemented by a package private subclass).
But the above annotation suggests that it is OK to provide a mapping function that provides StyxObservable subclasses. Perhaps the function signature needs to be just:
Function<? super T, StyxObservable<? extends U>> transformation
In fact this remark probably applies to all instances where this pattern is repeating.
There was a problem hiding this comment.
I think StyxObservable is an interface, isn't it? :) In this case, both signatures would accept any implementation of 'StyxObservable' (mmm, provided the declared type was for StyxObservable?) , but I think the one used by Kyle/rxJava would allow to pass Function<StyxCoreObservable, SecondParam> whereas your version only allows Function<StyxObservable, SecondParam> .
I would go for your proposed change and simplify the code as, like you said, we are not going to have different implementations of StyxObservable and, thus, I don't see a reason to restrict the type of the (input type of the) function (StyxCoreObservable instead of StyxObservable).
There was a problem hiding this comment.
Upon making the suggested change to flatMap, a lot of the scala Specs stop compiling. It's not completely clear to me why, but I think there may be some significance to the new signature style (as seen in Rx) beyond what we currently realise. I will continue investigating.
There was a problem hiding this comment.
I have investigated a little bit an the problem with Function<? super T, StyxObservable<? extends U>> is that it will only accept a function whose return type is verbatim StyxObservable<? extends U>.
This means that a function that returns StyxObservable<B> where B is a subtype of U will not be allowed.
For this reason we need the Function<? super T, ? extends StyxObservable<? extends U>>.
There was a problem hiding this comment.
Ah, of course, <StyxObservable<Integer>> is not compatible with <StyxObservable<? extends Number>> due to the type invariance of the first/outer parametrized type (<StyxObservable>). That´s what happens (to me) when reviewing code outside the IDE...
| @@ -103,4 +83,21 @@ public StyxCoreObservable(CompletionStage<T> future) { | |||
| return future; | |||
| } | |||
|
|
|||
| private static <U> Observable<? extends U> toObservable(StyxObservable<U> styxObservable) { | |||
There was a problem hiding this comment.
Do you see any advantage to the return type? It's usually considered a bad practice to include wildcard types in return types as, rather than providing more flexibility to the caller, it's just makes the code more bloated.
If we pass a StyxObservable U we are always going to return an Observable U and not any subclass of U.
BTW, T seems better than U if we want to be consistent.
There was a problem hiding this comment.
Hey @dvlato I assume you are referring to these guidelines?
https://docs.oracle.com/javase/tutorial/java/generics/wildcardGuidelines.html
There was a problem hiding this comment.
I didn't know that was in the Java tutorials! I thought it was usually quoted by the 'Effective Java' book.
There was a problem hiding this comment.
This one is easily fixed as it is private and doesn't break anything. As for which letters to use for type parameters, both Rx and java streams use T for input and R for output (where both are present), so it seems reasonable to use the same practice.
| @@ -62,31 +50,23 @@ public StyxCoreObservable(CompletionStage<T> future) { | |||
| return new StyxCoreObservable<T>(Observable.error(cause)); | |||
| } | |||
|
|
|||
| public <U> StyxObservable<U> map(Function<T, U> transformation) { | |||
| public <U> StyxObservable<U> map(Function<? super T, ? extends U> transformation) { | |||
There was a problem hiding this comment.
I'd rather change the return type name to R (instead of U) following Java 8 (e.g, https://docs.oracle.com/javase/8/docs/api/java/util/function/Function.html) and Observable's convention.
Maybe just copy the generic names from the Observable interface for the same methods?
The type parameters were overly strict.