2.x Remove Function from transformer interfaces to allow a single obj…

[dalewking]

This pull request removes Function from the super type of the various Transformer interfaces. While these are technically functions, the problem is that if they all extend Function then you cannot have a single object instance that can implement multiple Transformer interfaces. The goal is to be able to call a function that returns an object that can be passed to Observable.compose or Single.Compose and so on.

This was an issue with RxLifecycle project. See this for more info: trello-archive/RxLifecycle#39

[akarnokd]

We should avoid covariant return types (drop ? extends).

[dalewking]

It was there before in the code I changed, so I left it there. Whether it should be there or not is orthogonal to this request

[akarnokd]

This is also an opportunity to fix the signatures, saves 1 PR.

[dalewking]

I am not certain whether the ? extends should be there or not. I know that I have long had generic type problems with compose and RxLifecycle and am not sure whether the fault is RxJava or RxLifecycle. And the results vary between Java7, Java 8, and Kotlin regarding this.

See this discussion in RxLifecycle: trello-archive/RxLifecycle#58

[akarnokd]

There is no need to try-catch it if it doesn't declare throws Exception. So either each transformer should specify apply(...) throws Exception or these are not needed.

[dalewking]

It does throw exception see later commit

[akarnokd]

? extends should be removed as covariant return types are usually inconvenience to the consumer.

[akarnokd]

/cc @JakeWharton

[dalewking]

Another cleanup is that some of the compose methods use paramater name transformer and some use composer, should settle on one or the other.

[codecov-io]

Current coverage is 81.99% (diff: 100%)

Merging #4672 into 2.x will decrease coverage by 0.07%

@@                2.x      #4672   diff @@
==========================================
  Files           565        565          
  Lines         37399      37414    +15   
  Methods           0          0          
  Messages          0          0          
  Branches       5746       5746          
==========================================
- Hits          30694      30677    -17   
- Misses         4614       4648    +34   
+ Partials       2091       2089     -2   

Powered by Codecov. Last update 39a4e42...e4689b6

[ZacSweers]

What's the reasoning for the exception in the signatures?

[dalewking]

The reasoning for the exception was that the previous version where they implemented Function supported throwing exceptions so I wasn't trying to change the method signature (there was actually a test for it on one of the types which is why the first build failed).

For the more general reasoning for why all the functional interfaces in RxJava2 declare throws Exception see this section: https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#functional-interfaces

[ZacSweers]

Cool, thanks for the explanation!

[ZacSweers]

LGTM pending resolution of the covariant return type comment

[vanniktech]

Should not a unit test be added to guarantee this behavior?

[akarnokd]

It seems I have to do that myself.

[vanniktech]

How do you guys feel if all of the operators (CompletableOperator, FlowableOperator, ...) would change to this behavior too for consistency and for the same reasons @dalewking already stated.

[akarnokd]

They can be standalone, just don't forget the javadoc.

[dalewking]

I am also thinking about this for the "to" method by changing the XxxxxTransformer interfaces to be more general purpose and usable by both compose and to. Here is what I am suggesting as applied to Observable and if it sounds good to you I will do a PR:

The new ObservableTransformer:

/**
 * Interface to transform Observables, used for composing and "to" transformations.
 *
 * @param <Upstream> the upstream value type
 * @param <Result> the transformation result type
 */
public interface ObservableTransformer<Upstream, Result> {
    /**
     * Applies a function to the upstream Observable and returns a new result with
     * optionally different element type.
     * @param upstream the upstream Observable instance
     * @return the transformed result
     * @throws Exception in case the transformation throws, checked exceptions will be wrapped
     * into a RuntimeException
     */
    Result apply(Observable<Upstream> upstream) throws Exception;
}

compose and to methods:

@SchedulerSupport(SchedulerSupport.NONE)
public final <R> Observable<R> compose(ObservableTransformer<T, ObservableSource<R>> composer) {
        return wrap(to(composer));
}

@SchedulerSupport(SchedulerSupport.NONE)
public final <R> R to(ObservableTransformer<T, R> converter) {
    try {
        return converter.apply(this);
    } catch (Throwable ex) {
        Exceptions.throwIfFatal(ex);
        throw ExceptionHelper.wrapOrThrow(ex);
    }
}

[zhoutianxing]

yes

[dalewking]

After having tried implementing what I showed above and seeing how many changes that required in the unit test, I am not sure it is the best approach. It may be better to introduce interfaces like ObservableFunction<T, R> which signature wise is like Function<Observable<T>, R> but not extending Function that is used for the "to" method and ObservableTransformer extends that interface, where R is ObservableSource<DownStream>. Interested in feedback.