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

Observable.just(T value) and Observable.from(T t1) are confusing #1563

Closed
runningcode opened this issue Aug 9, 2014 · 41 comments
Closed

Observable.just(T value) and Observable.from(T t1) are confusing #1563

runningcode opened this issue Aug 9, 2014 · 41 comments
Labels
Milestone

Comments

@runningcode
Copy link
Contributor

Having Observable.just(T value) and Observable.from(T t1) is confusing to users of the API. It looks like calling Observable.from(T t1) calls Observable.just(T value) internally. The problem is that this method also exists: Observable.from(Iterable<? extends T> iterable). I found myself accidentally passing a list in to Observable.from(T t1)only to find that it took each item in that list and emitted it separately.

This becomes especially confusing when using generics. For example, suppose I have a class Foo<T> which calls Observable.from(T t1) internally. If I create class Foo like Foo<List<Bar>> what happens when it calls Observable.from(T t1) internally? Does it call Observable.from(T t1) or Observable.from(Iterable<? extends T> iterable)?

I suggest removing Observable.from(T t1) or renaming Observable.from(Iterable<? extends T> iterable) to Observable.fromIterable(Iterable<? extends T> iterable).

@samuelgruetter
Copy link
Contributor

In RxScala, we call it Observable.items instead of Observable.from (see docs). IMHO it would be nice to do the same in RxJava.

@benjchristensen
Copy link
Member

I'm not a fan of removing 'from(T)' this late in the game (almost to 1.0). Most of the time it is desired to itemize an Iterable. The 'just' operator is there for when it shouldn't be itemized.

/cc @headinthebox

@benjchristensen
Copy link
Member

If we do pursue this change I'd probably do ...

  • delete 'just'
  • leave 'from(T)'
  • change 'from(Iterable)' to 'fromIterable(Iterable)'

The thing I don't like about this is it will break most uses of 'from(Iterable)' that itemizes as most people want it to.

@benjchristensen
Copy link
Member

By the way, the reason why it is designed the way it currently is is because an Observable is intended for itemized sequences of data. It is rare that the desired output is Observable<Iterable> instead of Observable.

Thus in the Foo case it would correctly convert T into an itemized sequence, which is what "from" is for. If a scalar response is desired that is what "just" is for.

So if Foo should emit Observable<List> then use Observable.just rather than the more common Observable.from.

@benjchristensen
Copy link
Member

Any further comments or discussion on this? The key deterrent to changes here are:

  1. Massive breaking change to eliminate from(T) or from(Iterable i)
  2. The intended design of from is to itemize to Observable<T> and of just is to not itemize.

@abersnaze
Copy link
Contributor

I support a change. Methods that do different things should have different names. It's currently ambiguous if the call Observable.from(new ArrayList<T>()) should return Observable<T> or Observable<ArrayList<T>>. I'm partial to not having a raw from(...) and break them out into their own methods.

  • fromItems(T...)
  • fromFuture(Future<T>)
  • fromIterable(Iterable<T>)

and maybe 😃

  • fromEnumerable(Func0<Enumeration<T>>)

@headinthebox
Copy link
Contributor

I personally don't like fooXXX(XXX foo) names. That's why java has overloading. But if overloading does not work, we have no choice.

@benjchristensen
Copy link
Member

I also don't like fromXXX method names.

I agree that the current semantics are somewhat awkward and could have been designed better. However, the cost of changing this is not worth the massive break on almost all existing applications since from is such a commonly used method signature.

I don't think this change should be made right before hitting 1.0. Eventually when 2.x is done (not anytime soon) this could be cleaned up, but in well over a year of usage by many people, this issue is the first time I'm aware of this issue being been brought up, thus I don't think the confusion is significant enough to warrant a breaking change.

@runningcode
Copy link
Contributor Author

I would refer to Item 41 in Effective Java by Josh Bloch. Bloch warns to use overloading judiciously for this specific reason. Josh's advice would be to create methods such as @abersnaze suggested. Overloading in Java is broken.
I'll take a quote from the book for those who don't have access "A safe, conservative policy is never to export two overloadings with the same number of parameters"

@headinthebox
Copy link
Contributor

I don't buy "appeal to authority figure" arguments. Just saying.

@mattrjacobs
Copy link
Contributor

My 2c -

The current design is not correct, as described above. In particular, it's definitely surprising for a user that Observable.from(list1, list2) emits 2 lists, and Observable.from(list1) emits all elements of list1.

However, this seems like a severe cost in terms of migration for moderate-at-best benefit. I've been involved with keeping a large multi-language codebase up-to-date with RxJava as it evolves, and this migration would be a painful one.

My vote is to leave it as-is, and document the from(...) and just(...) Javadocs, as well as the Observable creation page with a clear explanation of how they are intended to be used.

@benjchristensen
Copy link
Member

/cc @zsxwing @akarnokd @jmhofer @mttkay @daveray @davidmoten @daschl @gorzell and anyone else who has an opinion to share.

@abersnaze
Copy link
Contributor

@headinthebox I'm not against overloads. Just overloads that do different things. There will still be plenty of overloads in my proposal:

  • fromFuture(Future<T>)
  • fromFuture(Future<T>, Scheduler)
  • fromFuture(Future<T>, long, TimeUnit)
  • fromFuture(Future<T>, long, TimeUnit, Scheduler)
  • fromItems(T)
  • fromItems(T, T)
  • fromItems(T, T, T)
  • fromItems(T, T, T, T)
  • fromItems(T, T, T, T, T)
  • fromItems(T, T, T, T, T, T)
    ...

@benjchristensen, @mattrjacobs: could 0.20 depreciate from(...) and friends and 1.0 delete to appease the naysayers?

@benjchristensen
Copy link
Member

It is not the design I'm disputing, it's how badly this breaks almost all apps. Is this problem (that no one has complained about for over a year) really so bad as to warrant the breakage?

Whether the methods are deleted in 0.20 or 1.0 doesn't matter, whenever it happens the break happens (though if we do this we'd deprecated in 0.20 and delete in 1.0).

@benjchristensen
Copy link
Member

If we do make a change ....

Current:

from(Future<? extends T>);
from(Future<? extends T>, long, TimeUnit);
from(Future<? extends T>, Scheduler);
from(Iterable<? extends T>);
from(Iterable<? extends T>, Scheduler);
from(T);
from(T, T);
from(T, T, T);
from(T, T, T, T);
from(T, T, T, T, T);
from(T, T, T, T, T, T);
from(T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T, T, T);
from(T...);
from(T[], Scheduler);
just(T);
just(T, Scheduler);

I would delete the just methods and then pursue something like this:

fromFuture(Future<? extends T>);
fromFuture(Future<? extends T>, long, TimeUnit);
fromIterable(Iterable<? extends T>);
fromArray(T[]);
from(T);
from(T, T);
from(T, T, T);
from(T, T, T, T);
from(T, T, T, T, T);
from(T, T, T, T, T, T);
from(T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T, T, T);

or maybe:

from(Future<? extends T>);
from(Future<? extends T>, long, TimeUnit);
from(Iterable<? extends T>);
from(T[]);
items(T);
items(T, T);
items(T, T, T);
items(T, T, T, T);
items(T, T, T, T, T);
items(T, T, T, T, T, T);
items(T, T, T, T, T, T, T);
items(T, T, T, T, T, T, T, T);
items(T, T, T, T, T, T, T, T, T);
items(T, T, T, T, T, T, T, T, T, T);

or:

from(Future<? extends T>);
from(Future<? extends T>, long, TimeUnit);
from(Iterable<? extends T>);
from(T[]);
fromItems(T);
fromItems(T, T);
fromItems(T, T, T);
fromItems(T, T, T, T);
fromItems(T, T, T, T, T);
fromItems(T, T, T, T, T, T);
fromItems(T, T, T, T, T, T, T);
fromItems(T, T, T, T, T, T, T, T);
fromItems(T, T, T, T, T, T, T, T, T);
fromItems(T, T, T, T, T, T, T, T, T, T);

@daschl
Copy link
Contributor

daschl commented Aug 12, 2014

I'm not fond of changing this, but if there are good reasons I'm okay with it. In my whole codebase I never ran across an issue with that, and sometimes I even explicitly use the "just" instead of a from(T) to make it clear I'm returning just one item, basically.

I'm good if we get to something more clear, but on the other hand as @benjchristensen said it will break a good deal of libraries I'm sure (fixing it is not very hard, though).

@benjchristensen strictly speaking, are we done with cleaning the API yet with 0.20 or is there still opportunity for 1.0? SUBJ: in my opinion we gain too less and risk too much annoyance with breaking this, since its just very minor API nuances?

@daveray
Copy link
Contributor

daveray commented Aug 12, 2014

Not a big deal for Clojure-land. We'd just change rx/return to use fromItems(T) instead of just and rx/seq->o to use fromIterable() or whatever. We (at least me and my team) don't use any of the other overloads.

@benjchristensen
Copy link
Member

After 0.20 we will immediately move to 1.0-RC1 with the only change being the deletion of deprecated methods/types. Therefore, API cleaning and changing should be done, with only outstanding work shown here: https://github.com/Netflix/RxJava/issues?q=is%3Aopen+is%3Aissue+milestone%3A0.20

Some small breaking changes have been done in 0.20, but nothing on major APIs (removal of pivot and deprecation of mergeMap which duplicated flatMap). All major work of 0.20 has been additive (backpressure).

@samuelgruetter
Copy link
Contributor

I'd take @benjchristensen's second proposal, but keep the just methods, since they don't "hurt":

from(Future<? extends T>);
from(Future<? extends T>, long, TimeUnit);
from(Iterable<? extends T>);
from(T[]);
items(T);
items(T, T);
items(T, T, T);
items(T, T, T, T);
items(T, T, T, T, T);
items(T, T, T, T, T, T);
items(T, T, T, T, T, T, T);
items(T, T, T, T, T, T, T, T);
items(T, T, T, T, T, T, T, T, T);
items(T, T, T, T, T, T, T, T, T, T);
just(T);
just(T, Scheduler);

So the only change would be from(T*) --> items(T*). I imagine that from(T*) is only used rarely (except for toy examples), so the change should not break too much code.

@mttkay
Copy link
Contributor

mttkay commented Aug 12, 2014

I think we should keep just, first because it adds clarity on the call site, and second because of the problem @runningcode mentioned with from(Iterable) exploding lists. We use observables a lot to emit items in batches/pages, since that's how they're returned from the server, and most of the time there's no value in exploding a page into individual emissions, since we'd have to turn them back into batches again. This is where just helps, since it takes the list and emits it as is.

To summarize:
I'd vote to keep the current API, with from(Iterable<T>) for conversion to Observable as per the duality principle, just(T) to emit single values. If anything, I'd drop the from(T) overload that takes a single argument, since there's a chance it won't do what you expect it to do.

@davidmoten
Copy link
Collaborator

I also like @benjchristensen's second proposal but would suggest that
items(T) should be item(T) to avoid the ambiguity of items(<Iterable<Foo>>)

@zsxwing
Copy link
Member

zsxwing commented Aug 13, 2014

I don't like the ambiguity of from(T) and from(Iterable<T>). From the discussion, I think the question is when to fix it. IMO, I prefer starting it early.

@NiteshKant
Copy link
Member

My $0.02

The current design is certainly confusing and could be changed. However, the tax on users that this backward incompatible change would bring because of the shear intrusiveness of Observable.just() and Observable.from() in our codebase does not justify this change.

I would vote for keeping the current API at this point.

benjchristensen added a commit to benjchristensen/RxJava that referenced this issue Aug 14, 2014
@benjchristensen
Copy link
Member

Please review proposed changes in #1576. Ignore the deluge of unit test changes and focus on Observable.java.

I deprecated the from(T) methods, added item(T) and items(T*), left just alone. I also deprecated the unnecessary from(*, Scheduler) overloads where subscribeOn can just be used.

Nothing should break in 0.20 with this. The deprecated methods will be removed in 1.0RC1 with all other deprecated methods.

If there are naming or signature changes to debate, let's make sure to get it correct now so please review and speak up.

@runningcode
Copy link
Contributor Author

I do like these changes. I think it will be less confusing for users. However, I think it is a bit confusing to keep the just methods. I believe a typical user would study the javadoc for the just and item and find that they are very similar and wonder in what situations they should use one over the other. I did this the first time used just and from. The user would then have to look at the source to see that item calls just.

This could also be fixed by changing the JavaDoc to say that item calls just. Though, I would prefer removing just.

@benjchristensen
Copy link
Member

What is better for a single item, Observable.just(T) or Observable.item(T)?

For multiple should it be Observable.just(T, T) or Observable.items(T, T)

@mttkay
Copy link
Contributor

mttkay commented Aug 14, 2014

I would prefer just, and it already exists so people are familiar with
what it does.

@benjchristensen
Copy link
Member

I think I do too. It would mean we stick with 'from' and 'just' instead of introducing yet another name to discover.

The 'just' name also conveys it will emit "just" what it is given.

@davidmoten
Copy link
Collaborator

I like the consistency of item and items. 'just' for multiple seems odd.
On 14 Aug 2014 21:33, "Ben Christensen" notifications@github.com wrote:

I think I do too. It would mean we stick with 'from' and 'just' instead of
introducing yet another name to discover.

The 'just' name also conveys it will emit "just" what it is given.


Reply to this email directly or view it on GitHub
#1563 (comment).

@benjchristensen
Copy link
Member

Current
from(Future<? extends T>);
from(Future<? extends T>, long, TimeUnit);
from(Future<? extends T>, Scheduler);
from(Iterable<? extends T>);
from(Iterable<? extends T>, Scheduler);
from(T);
from(T, T);
from(T, T, T);
from(T, T, T, T);
from(T, T, T, T, T);
from(T, T, T, T, T, T);
from(T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T, T);
from(T, T, T, T, T, T, T, T, T, T);
from(T...);
from(T[], Scheduler);
just(T);
just(T, Scheduler);
Proposal A

NOTE: This will break from(T) usage but not just(T).

from(Future<? extends T>)
from(Future<? extends T>, long, TimeUnit)
from(Future<? extends T>, Scheduler)
from(Iterable<? extends T>)
from(T[])
item(T)
items(T, T)
items(T, T, T)
items(T, T, T, T)
items(T, T, T, T, T)
items(T, T, T, T, T, T)
items(T, T, T, T, T, T, T)
items(T, T, T, T, T, T, T, T)
items(T, T, T, T, T, T, T, T, T)
items(T, T, T, T, T, T, T, T, T, T)
just(T)
Proposal B

NOTE: This will break from(T) usage but not just(T).

from(Future<? extends T>)
from(Future<? extends T>, long, TimeUnit)
from(Future<? extends T>, Scheduler)
from(Iterable<? extends T>)
from(T[])
just(T)
just(T, T)
just(T, T, T)
just(T, T, T, T)
just(T, T, T, T, T)
just(T, T, T, T, T, T)
just(T, T, T, T, T, T, T)
just(T, T, T, T, T, T, T, T)
just(T, T, T, T, T, T, T, T, T)
just(T, T, T, T, T, T, T, T, T, T)
Proposal C

NOTE: This will break from(T) and just(T).

from(Future<? extends T>)
from(Future<? extends T>, long, TimeUnit)
from(Future<? extends T>, Scheduler)
from(Iterable<? extends T>)
from(T[])
item(T)
items(T, T)
items(T, T, T)
items(T, T, T, T)
items(T, T, T, T, T)
items(T, T, T, T, T, T)
items(T, T, T, T, T, T, T)
items(T, T, T, T, T, T, T, T)
items(T, T, T, T, T, T, T, T, T)
items(T, T, T, T, T, T, T, T, T, T)

I prefer B as I find the item/items differentiation awkward, and I prefer not breaking usage of just.

Any other proposals?

I'd like to make a decision on this within the next 24 hours so I can release the next RC.

Please weigh in ...

@headinthebox
Copy link
Contributor

+1 for B (agree item/items is super awkward)

benjchristensen added a commit to benjchristensen/RxJava that referenced this issue Aug 14, 2014
See ReactiveX#1563 for discussion.
benjchristensen added a commit to benjchristensen/RxJava that referenced this issue Aug 14, 2014
See ReactiveX#1563 for discussion.
@runningcode
Copy link
Contributor Author

+2 for B

@benjchristensen benjchristensen added this to the 0.20 milestone Aug 14, 2014
@mattrjacobs
Copy link
Contributor

+3 for B

@vadims
Copy link
Contributor

vadims commented Aug 14, 2014

+1 for B

@pasviegas
Copy link

+1 B

@mttkay
Copy link
Contributor

mttkay commented Aug 14, 2014

B

@benjchristensen
Copy link
Member

Thanks everyone for your votes. I'm merging the pull request with option B.

@benjchristensen
Copy link
Member

Merged #1576.

The ambiguity will still exist in 0.20 due to the deprecated methods. In 1.0 the deprecated methods will be deleted.

@jbripley
Copy link
Contributor

In light of this change, would it make sense to rename RxScala's items(T_) to just(T_) as well?

@headinthebox
Copy link
Contributor

Go ahead.

@jbripley
Copy link
Contributor

Done in #1586

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

No branches or pull requests