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

3.x: Proposal: ObservableConverter interface and friends #5654

Closed
ZacSweers opened this issue Oct 9, 2017 · 17 comments
Closed

3.x: Proposal: ObservableConverter interface and friends #5654

ZacSweers opened this issue Oct 9, 2017 · 17 comments

Comments

@ZacSweers
Copy link
Contributor

For the compose() operators, ObservableTransformer was introduced to allow for classes to implement multiple transformer interfaces for composite implementations. I'd like to propose doing the same for the to() operator by introducing ObservableConverter (and corresponding ones for others). This would allow for composite converters as well.

If you're open to this, I can contribute a PR!

@ZacSweers ZacSweers changed the title 2.x Proposal: ObservableConverter interface and friends 2.x: Proposal: ObservableConverter interface and friends Oct 9, 2017
@akarnokd akarnokd added the 2.x label Oct 9, 2017
@akarnokd
Copy link
Member

akarnokd commented Oct 9, 2017

That would break binary compatibility and otherwise to(Function<? super Observable<T>, R>) isn't that much of a type mess as lift or compose is.

@ZacSweers
Copy link
Contributor Author

This would be an alternative to, not a replacement for. I'm not sure what type mess means, the larger issue is that you can't have something implement Function multiple times with different generics

@akarnokd
Copy link
Member

akarnokd commented Oct 9, 2017

A to overload will lead to type ambiguity when one writes to(f -> somefunc(f)).

@ZacSweers
Copy link
Contributor Author

ah, good point. Would you be against a different name? Like convertTo? Or would this be more of an RxJava 3.0 discussion?

@akarnokd
Copy link
Member

akarnokd commented Oct 9, 2017

Yes, 3.x API cleanup can include this type of change. I otherwise always have separate converters for the base reactive classes.

/cc @JakeWharton, @vanniktech, @artem-zinnatullin

@vanniktech
Copy link
Collaborator

I remember we had the discussion about to() already in 2.0.0-RC days. The closed I could find is - #4672 - if I recall correctly it was decided to not change the to operator.

I don't have a strong opinion about changing it in 3.x though.

@artem-zinnatullin
Copy link
Contributor

In general, I'd like RxJava to have as less interfaces as possible, especially if generic ones like Function can be used instead.

But Java limitations kinda force us to have them here. So I'm ok if 3.x will have dedicated *Converter interfaces.

@akarnokd akarnokd added 3.x and removed 2.x labels Oct 18, 2017
@akarnokd akarnokd changed the title 2.x: Proposal: ObservableConverter interface and friends 3.x: Proposal: ObservableConverter interface and friends Oct 18, 2017
@akarnokd
Copy link
Member

Retagged as 3.x.

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Nov 16, 2017

I'd like to lean on this for a 2.x consideration again, seeing as 3.x discussion seems to be settling on "not necessary anytime soon".

Proposal, to protect against ambiguity, would be to add a new convert or convertTo method, which retains the semantics while introducing the new interfaces to allow for composition.

The old to() methods could optionally be deprecated too to avoid confusion

@akarnokd
Copy link
Member

Project reactor chose the as() naming. It's short, which I like, and could be added without deprecating to.

public <R> R as(ObservableConverter<T, R> converter);
public <R> R as(FlowableConverter<T, R> converter);
public <R> R as(SingleConverter<T, R> converter);
public <R> R as(MaybeConverter<T, R> converter);
public <R> R as(CompletableConverter<R> converter);

@ZacSweers
Copy link
Contributor Author

I like it! Should I send a PR?

@akarnokd
Copy link
Member

Sure.

@ZacSweers
Copy link
Contributor Author

Done in #5729

@akarnokd
Copy link
Member

This has to be still resolved in 3.x where I suggest having only to() with the custom interfaces just introduced.

@akarnokd akarnokd reopened this Nov 19, 2017
@ZacSweers
Copy link
Contributor Author

Sounds good

@phatnhse
Copy link

phatnhse commented Jan 8, 2019

Hi guys, I have a concern which related to this ticket. Here is my code:

   inline fun <reified T> withSchedulers(subscribeOn: Scheduler, observeOn: Scheduler): T {
       when (T::class) {
           FlowableTransformer::class -> return FlowableTransformer<Any, Any> {
               it.subscribeOn(subscribeOn).observeOn(observeOn)
           } as T
           ObservableTransformer::class -> return ObservableTransformer<Any, Any> {
               it.subscribeOn(subscribeOn).observeOn(observeOn)
           } as T
           SingleTransformer::class -> return SingleTransformer<Any, Any> {
               it.subscribeOn(subscribeOn).observeOn(observeOn)
           } as T
           CompletableTransformer::class -> return CompletableTransformer {
               it.subscribeOn(subscribeOn).observeOn(observeOn)
           } as T
           MaybeTransformer::class -> return MaybeTransformer<Any, Any> {
               it.subscribeOn(subscribeOn).observeOn(observeOn)
           } as T
       }
       throw IllegalArgumentException("not a valid Transformer type")
   }

Are there any ways to reduce the boilerplate code? or any better approaches to achieve this? Or just write 5 functions for 5 types, something like to operator above?

Thanks.

@akarnokd
Copy link
Member

Closing via #6514.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants