Skip to content

Completable.subscribe(onError, onComplete) design issue #3851

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

Closed
artem-zinnatullin opened this issue Apr 11, 2016 · 6 comments
Closed

Completable.subscribe(onError, onComplete) design issue #3851

artem-zinnatullin opened this issue Apr 11, 2016 · 6 comments

Comments

@artem-zinnatullin
Copy link
Contributor

This particular overload of Completable.subscribe() looks out of design in compare to other RxJava classes like Observable and Single.

Spent ~5 minutes trying to understand why such code didn't compile:

Completable
  .fromAction(() -> doSomething())
  .subscribe(
    () -> ui.success(), 
    error -> ui.error(error)
  );

And the reason is because this overload accepts error handler first and complete handler second.

I do understand that it comes from Observable.subscribe(onNext, onError, onComplete).

But in compare to many other overloads like:

  • Observable.subscribe(onNext, onError)
  • Single.subscribe(onSuccess, onError) // this one has very similar semantic to target overload.
  • Observable.subscribe(onNext)
  • Single.subscribe(onSuccess)

Error handler is never first parameter in Observable.subscribe() and Single.subscribe().

Another point is that compiler error displayed in IDE makes it even worse:
screen shot 2016-04-12 at 00 35 50


Completable is still in @Experimental, so, we can change this signature as we want. We can @Deprecate this overload and add "better" alternative and then delete deprecated overload after one-two releases.

@akarnokd
Copy link
Member

The design pattern was:

subscribe(Action0 onComplete)
subscribe(Action0 onComplete, Action1<Throwable> onError)

@artem-zinnatullin
Copy link
Contributor Author

@akarnokd what do you mean by

was

?

@akarnokd
Copy link
Member

I mean that is your suggestion but the current is swapped for case 2 because it is in the same order as the 3 arg Observable.subscribe minus onNext. Not sure which order feels more consistent with the other classes.

@artem-zinnatullin
Copy link
Contributor Author

Right, I pointed that in the issue description, but we do have Single.subscribe(onSuccess, onError) even though it's also Observable.subscribe(onNext, onError, onComplete) minus onNext.

As said before: no other overloads of subscribe() in any classes accept onError first -> makes Completable inconsistent.

@akarnokd
Copy link
Member

Okay. Since Completable is experimental, lets swap the arguments as you suggested.

@artem-zinnatullin
Copy link
Contributor Author

Oh great, will do PR soon!

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

2 participants