-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Naming of Create/Bind/Operator Functions and Types #775
Comments
How about Observable<T> o = Observable.<T>with(operatorA).with(operatorB).with(operatorC); |
This is actually an instance of map since it takes (Operator->Operator |
For this one, I actually like keeping Func1; otherwise I'd have to unfold the synonym. That is because I remember
That is easier than
|
I like this one. With
I need to mentally translate too much. |
Can't we just make Observer implement Subscription? Less types is better. |
This would be my choice. As I said not sure the last one works.
|
Note that |
What would be a good word for "before". Since "bind" is contravariant, it chains the transformations in reverse order. In my younger days when Algol 68 was all the rage if then else fi, while do od, case esac, I would propose neht, but I am sure that this would not fly in 2014 ;-) |
I'm not sure if I like having Observer implementing a Subscription. Now you can have the same Observer instance subscribed to more than one Observable if you so wish, and what you unsubscribe is the Observable <-> Observer association. I guess it would be okay if Observer were to become only an internal type. Also, there might be some strangeness with Subjects. |
After playing with this last night I'm beginning to like the idea of changing Changing from an interface to an abstract class would minimize the breakiness to just forcing everyone to switch from Come to think of it are there any other |
I too don't think making 1) Conflates ResponsibilityWhen using an What does implementing This is what we (users) would have to implement: new Observer<String>() {
@Override
public void onCompleted() {
}
@Override
public void onError(Throwable e) {
}
@Override
public void onNext(String args) {
}
public final void unsubscribe() {
// what to do here?
}
} In most cases I don't know what the Contrast this with the current public final Subscription subscribe(Observer<? super T> observer) {
... simplified ...
Operator<T> op = Operator.create(observer, new CompositeSubscription());
subscribe(op);
return op;
} This is not logic that most users of Rx should ever have to worry about. I feel that 2) Major Breaking ChangeThis would be a massive breaking change to anyone using RxJava. There is no mechanism for making it backwards compatible or providing a deprecation strategy. |
I'm not sure if you were replying directly to my statement. What about making |
I was replying to @headinthebox but it applies to yours as well (we posted within minutes of each other).
Perhaps, but it's still a pretty major breaking change. I can see some limited benefit in an The problem obviously is that we now have 2 ways of subscribing: public Subscription subscribe(Observer<? super T> observer) and public void subscribe(Operator<? super T> o) So the real question is whether we are willing to have a massive breaking change where we eliminate all of the If we were starting from scratch I wouldn't have the |
Another problem I just considered ... Thus, if we make |
What exactly is wrong with the name |
Putting the MI part aside, I a do not agree with I see absolutely nothing wrong with that. Now most of the time people do not even pass observers directly but just the onNext, onError, or onCompleted functions to most of the time they won't even notice. |
http://en.wikipedia.org/wiki/Operator_(programming) An operator is typically a Function3. |
I would say, keep just one subscribe
Instead of returning void there is no harm in returning the (augmented) subscription in both bases. |
That is not a smart idea anyway, not sure if that would always guarantee that calls to OnNext are serialized if they get called from tow independ observables. |
This is the signature that starts the void subscribe(Observer&Subscription o) The Also, it must have the private final CompositeSubscription cs;
public final void add(Subscription s) {
cs.add(s);
}
public final void unsubscribe() {
cs.unsubscribe();
}
public final boolean isUnsubscribed() {
return cs.isUnsubscribed();
} Thus, a user can not just pass in an empty This is why Operator op = Operator.create(observer, new CompositeSubscription());
subscribe(op) If Thus we would now have users implementing an subscribe(new ObserverThatExtendsSubscription<String>() {
@Override
public void onCompleted() {
}
@Override
public void onError(Throwable e) {
}
@Override
public void onNext(String s) {
// receive data
if(s == null) {
unsubscribe();
}
}
public void unsubscribe() {
// what do I do here?
// I want to do this =>
parentSubscription.unsubscribe();
// but there is no hook to a "parentSubscription"
}
} The problem is that if I'm implementing just an interface, there is no "parentSubscription" passed in that I can do something with. Thus, what would someone do inside the The only way I see to make this work is to replace the But it won't work with Using this abstract class would work exactly like the current subscribe(new OperatorOrObserver<String>() {
@Override
public void onCompleted() {
}
@Override
public void onError(Throwable e) {
}
@Override
public void onNext(String s) {
// receive data
if (s == null) {
unsubscribe(); // this will now work
}
}
}); This would work great, except for (1) it's a massive breaking change and (2) Subjects would all break. |
Agreed. But it's additional flexibility that comes from separating those ideas. And the Another benefit is that now a subscription can be shared between a chain of observers, whereas otherwise each operator would have to cancel the parent observer, which in turn would cancel its parent, and so forth. |
One way around that would be to make it have a toObservable method. |
Talking with @headinthebox and @abersnaze we have discussed taking these changes all the way instead of part-way as done so far and doing the following: Change // Observer & Subscription
public abstract class Observer<T> implements Subscription {
public static create(onNext);
public static create(onNext, onError);
public static create(onNext, onError, onCompleted);
abstract void onNext(T);
abstract void onError(Throwable);
abstract void onCompleted();
final add(Subscription);
final unsubscribe();
final boolean isUnsubscribed();
} This would effectively be the same class as is currently Then Subject<T> extends Observer<T> {
public Observable<T> toObservable();
} This would allow The benefit of this is:
Regarding the naming of Thus it will be: // lift
public <R> Observable<R> lift(final Func1<Observer<? super R>, Observer <? super T>> cf);
... or ...
public <R> Observable<R> transform(final Func1< Observer <? super R>, Observer <? super T>> cf); This means version 0.17 will be a significant breaking release. We will leverage this to do all of the breaking changes here including removing deprecated methods and changing |
Would it make sense for Observer to extend from CompositeSubscription, as it will have those semantics? Being able to do Observer.create({ }).add(Observer.create{..}) still feels a bit weird. I wonder if this approach is just due to a limitation of Java's type system, as we can't express |
I don't think we want to expose the
It looks odd like that but there are actually use cases where I think that's more-or-less what we'll do (zip, groupBy, merge) albeit with a little more code involved and not directly chained like that. There are always things we can do that shouldn't be done, such as this infinite loop with a Subject<String, String> s = PublishSubject.create();
s.subscribe(s); This fits into the "don't do that" category. Similar to
I don't know regarding this specific decision. We are limited in our implementation design though due to things such as lack of extension methods which drives us to use |
As per discussion at ReactiveX#775 (comment)
As per decision at ReactiveX#775 (comment)
Here is my branch working on this refactor as a preview of the changes: https://github.com/benjchristensen/RxJava/commits/lift-observer The biggest issue I now have is figuring out how to make the unit tests work since |
I don't quite understand why Observer and Subscription needs to be merged this way. I got pretty "far" with one of the previously suggested structures: private final Action2<Observer<T>, CompositeSubscription> onSubscribe;
private Obsurvable(Action2<Observer<T>, CompositeSubscription> onSubscribe) {
this.onSubscribe = onSubscribe;
}
public Subscription subscribe(Observer<T> obsurver) {
CompositeSubscription token = new CompositeSubscription();
onSubscribe.call(obsurver, token);
return token;
}
public Subscription subscribe(Observer<T> obsurver, CompositeSubscription token) {
onSubscribe.call(obsurver, token);
return token;
}
public static <T> Obsurvable<T> create(Action2<Observer<T>, CompositeSubscription> onSubscribe) {
return new Obsurvable<>(onSubscribe);
}
public <U> Obsurvable<U> bind(Func2<Observer<U>, CompositeSubscription, Observer<T>> binder) {
return new Obsurvable<>((o, t) -> onSubscribe.call(binder.call(o, t), t));
} This way, both |
It definitely works on the previous signatures. I was arguing for leaving
In other words, specifically avoiding methods like you show: (Observer<T> obsurver, CompositeSubscription token)
(Action2<Observer<T>, CompositeSubscription>)
(Func2<Observer<U>, CompositeSubscription, Observer<T>>) That is where I started this journey at, and it works just fine as you say. Making (Observer<T> obsurver)
(Action1<Observer<T>)
(Func1<Observer<U>, Observer<T>>)
It's quite nice now that any
Yes. This last set of changes is purely one of semantics and signature design. The change in functionality is already what was merged into master that added the |
Apparently, I'm too late to the party (just seen the 0.17 release notes preview), but I wanted to say it anyway: the mathy-sciency-geeky |
I understand this sentiment. I'm torn on this one but there is a principle that pushed us towards using Part of the goal for Rx is to be consistent across platforms and languages which is why we work with @headinthebox and @mattpodwysocki to try and stay as close as we can. As we have pursued this path with Another aspiration is to not try and redefine what operators and functions mean. If we are using capabilities from "functional programming" or "math" it is our perspective that it is better for us to learn and use the proper names for things so we can speak the same language. Take the Last, we have avoided adding new types as much as possible and let the functional interfaces stay as such in most places such as All that said ...
... what specifically puts you off about this? Despite not liking the concept of alias methods, I'm not against having one if it is truly a blocker for users. Nor is the name |
Ben, first let me say that I very much appreciate your detailed explanation. I beg to differ, though. With RxJava, you are a bit in a special position, as you are bringing Rx to masses of Java programmers (which is a big deal itself), and you are also innovating on Rx at the same time (this What I've seen is that APIs in mainstream-ish languages tend to name things differently from the computer science "upstreams". Regarding |
@Ladicek Your make a strong argument and I'm willing to concede and make a change that benefits ease of adoption and comprehension. I'd like others to weigh in on this ... so /cc @samuelgruetter @jmhofer @zsxwing @mairbek @mattrjacobs @abersnaze @akarnokd @michaeldejong @mttkay @JakeWharton @loganj @adriancole The proposal is to change from: public <R> Observable<R> lift(final Func1<Subscriber<? super R>, Subscriber<? super T>> lift) to public <R> Observable<R> chain(final Func1<Subscriber<? super R>, Subscriber<? super T>> chain) The use is for custom operator chaining such as: Observable<String> os = observable_of_integers.chain(TAKE_5).chain(MAP_INTEGER_TO_STRING); More information about the background on this can be found at #802 in summary form. |
@benjchristensen Thanks for asking. For what it's worth, I disagree with @Ladicek's reasoning, if not the proposed change. There are a number of good criteria for choosing a function name, but whether or not it's "scary" because it has the whiff of math about it doesn't strike me as one of them.
That said, |
Thanks for your input @loganj I appreciate you taking the time to weigh in and you make good points. |
Thanks @loganj for your input. I agree that "it looks scary" is a lousy argument. However, it looks scary enough to make me join this discussion. Don't underestimate the power of fear :-) Anyway, let me rephrase an argument I'm trying to make here. I argue that it's common for mainstream to have a different terminology from the math/computer science origins. I hope you will agree with me on that (we don't necessarily have to agree on whether it's a good thing or not). And I argue that we shouldn't deviate from the common practice. One other example of importance of good naming. Dart's core library (dart:async) has a class called Stream, which is essentially Observable, and they have these two methods called |
I think this is settled. |
yup. |
Changes are being done in #770 as a result of discussion in #746 that add a new
bind
operator and change thecreate
signature to inject aSubscription
instead of returning it. The reasons for this change can be seen at #746.The names have thus far been left as is or as raw functions while achieving the right functionality. Before releasing and becoming part of the published API I'd like to finalize the names (some of which have been disputed).
1) "source" Function
This is the single function that exists inside an
Observable
that is executed upon subscription (whensubscribe
is invoked).The previous function was called
OnSubscribeFunc<T>
.We could just keep the
Action1
signature but the generics are obnoxious to remember, type and code-complete. Having a specific type is for ease of use.This will be the most used of the types discussed in this issue as all
Observable.create
usage will involve this type. Most users of RxJava will useObservable.create
.A possibility is:
or it could more explicitly (and like
OnSubscribeFunc
) be:What other names could this be?
2) "bind" or "lift" Method
The new method added to
Observable
has a name and signature like this:The intent is to perform function composition and return a new
Observable
representing the composition.Since my computer science, lambda calculus etc is not the greatest, I was calling this
bind
but after discussion with others it seems this is probably better calledlift
as per functional programming principles.That said, there is nothing requiring us to use names such as these.
The intent of this method is to chain together operators for transforming, combining, filtering and operating on data flowing through an
Observable
and each time it's chained it returns a newObservable
.For example:
Typically this would be used like this:
Here are possible names:
What is the correct name for this?
3) "bind" or "lift" Function
The function that the currently named
bind
takes is:This could be left as is and will generally only be used by people implementing operators. Thus, it is not in the "common path" for using Rx.
However, it could benefit from the clearer communication of a specific type and name.
Possibilities include:
Should we give this a specific type and name? If so, what should it be?
4) Operator class
As part of the new
bind
andcreate
functionality that injectsSubscriptions
into the source function, a new typeOperator
was created that combinesObserver
andSubscription
.Is
Operator
the correct name? If not, what should it be called?Possible Outcomes
or
or
etc
My intent is to solidify the naming before releasing 0.17.0 as these are not easy to change once released.
Please provide your suggestions and reasoning either to support one of the options above or for something new.
Thank you!
The text was updated successfully, but these errors were encountered: