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

Added experimental to() method for fluent type conversions. #2971

Closed
wants to merge 1 commit into from

Conversation

akarnokd
Copy link
Member

See #2793.

@abersnaze
Copy link
Contributor

I second this PR.

@stealthcode
Copy link

I would prefer if the Func1 were from OnSubscribe to O. There isn't really any reason that a conversion func should have to call subscribe. I would really like to build on top of this. I'll post a link with more examples shortly.

@stealthcode
Copy link

I have been working on a proof of concept using a similar mechanism to the to method. The API actually defines a class that takes an OnSubscribe<T> and returns an O.

public interface MonoConversion<O, T> {
    public O convert(OnSubscribe<T> onSubscribe);
}

I would really like to be able to use this to method in a similar way. Here is a link to the project and an example implementation of a conversion.

@akarnokd
Copy link
Member Author

to is an instance method whereas your convert method looks like a factory method without any relation to an Observable.

@stealthcode
Copy link

The MonoConversion interface is alpha equivalent with the Func1 interface as it has 1 method (call vs convert). The POC project linked has an observable class that exposes a method extend that takes a MonoConversion but it would be pretty much equivalent to the Func1. My only concern with this pull request was that your Func1 took an Observable<T> and I think it would be better to take an OnSubscribe<T> in order to make the method agnostic as to the type of observable. This would enable projects with their own implementations of the to method to accept the same underlying OnSubscribe and have general applicability across any kind of observable that uses an OnSubscribe.

@akarnokd
Copy link
Member Author

I don't see why you'd want access to the OnSubscribe inside an Observable.

@stealthcode
Copy link

Sorry, let me try to start from the top. I am trying to write an API for an extensible Observable that allows for more functionality than member methods and one-off lifted operators. I intend to use the to() method to convert from one kind of observable to another kind. For instance, convert from an rx.Observable class to something like a rx.TemporalObservable for part of the operator chain.

So if someone were to write a class Observable that followed the same interface as the rx.Observable (and at some point its feasible that there might be an interface to uphold an Observable contract) then the to member method on the observable would accept a Func1<rx.Observable<T>, R> which does not allow for anything else than an rx.Observable. The Observable as it is today carries with it much more functionality than a simple rx.OnSubscribe<T>.

This also means that in order to use that conversion function for any other kind of observable (ParallelObservable, BiObservable, TemporalObservable, etc.) then someone would have to use a shim class to subscribe to the Observable or the function would have a concrete import to the Observable variant class so that it can reach in to access the this.onSubscribe func directly. However, if the derived observables were to both agree to use an underlying OnSubscribe func then any other Observable class would be able to use the (much lighter weight) OnSubscribe func and get immediate reuse out of any conversion function written for any observable.

@akarnokd
Copy link
Member Author

if the derived observables were to both agree to use an underlying OnSubscribe func then any other Observable class would be able to use the (much lighter weight) OnSubscribe func and get immediate reuse out of any conversion function written for any observable.

I don't think this is even doable. For example the BiObservable would use a BiOnSubscribe<T> with a single method of call(BiSubscriber<? super T>) which has the onNext(A a, B b) method. The types are incompatible with OnSubscribe.

In addition, Observable is the base class for fluent operations with final methods and from API perspective, it is unfeasible and unnecessary to extract an interface for these methods: interface evolution isn't really possible in pre-Java 8. Even if both Observable and BiObservable have common method signatures, their non-zero parameter subscribe() can't be shared and thus can't be consumed independently of the underlying type.

@stealthcode
Copy link

To convert from an Observable to a BiObservable you have to provide a Subscriber that can be subscribed to the observable. The SingleToDyadOperator shows how this is done and can illustrate how this works. The operator extends Func1<DyadSubscriber<? super R0, ? super R1>, Subscriber<T>>. That is to say that it emits an rx.Subscriber that can be subscribed to the rx.Observable. Converting from an Observable would work in a similar way. I'm not sure that you understand the point that I am making.

@akarnokd
Copy link
Member Author

Still I don't see the point. You can call subscribe or unsafeSubscribe with a subscriber in the callback of to, where in the latter case you need to call unsubscribe() manually.

@stealthcode
Copy link

I'm fine with having a conversion function call the subscribe method. This makes a lot of sense in order to preserve the unsubuscribe on terminal event and the hooks callsites. However I don't want for a conversion function to have access to the full kitchen sink of the Observable. This kind of api is open for abuse.

@stealthcode
Copy link

@akarnokd thanks for hearing my feedback. Would you like for me to try and implement this?

@akarnokd
Copy link
Member Author

"Code wins arguments", so if you show me the code, I might be convinced.

@benjchristensen
Copy link
Member

@stealthcode Do you have examples to show? I'm leaning towards merging this since it is marked as @Experimental and it seems very clean and usable.

@stealthcode
Copy link

@benjchristensen yes please take a look at #3082. I had to make package visible static method encapsulating the work of Observable.subscribe in order to expose the subscribe behavior for OnSubscribe values that are not this.onSubscribe.

@akarnokd
Copy link
Member Author

I'm closing this PR since #3082 has been merged (which I find over-convoluted in contrast to this PR).

@akarnokd akarnokd closed this Aug 12, 2015
@stealthcode
Copy link

@akarnokd I really do want to hear from you regarding implementation and design decisions. You had no comments regarding convoluted implementation before the pull request was merged. Do you want to comment or open an issue?

@akarnokd akarnokd deleted the ObservableConvertTo branch September 9, 2015 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants