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

1.x: Add Completable.doOnCompleted and deprecate Completable.doOnComplete #3701

Merged
merged 1 commit into from
Feb 14, 2016

Conversation

zach-klippenstein
Copy link

Closes #3700.

@akarnokd
Copy link
Member

Wait, what? I thought you wanted to add Completable.doOnCompleted(). I see no reason to change the established naming of Observable.doOnCompleted(). In fact, for consistency, I'd rather prefer adding Completable.doOnCompleted.

@zach-klippenstein
Copy link
Author

That makes sense. The only reason I did it this way is because on the 2.x branch both Observable and Completable use doOnComplete().

@akarnokd
Copy link
Member

Yes, those follow the reactive-streams convention and Completable started out as a 2.x addition. In the meantime, I'll fix that test failure.

@zach-klippenstein
Copy link
Author

Changed Completable instead, updated tests to match.

@akarnokd
Copy link
Member

👍

@zsxwing zsxwing changed the title 1.x: alias Observable.doOnCompleted to doOnComplete to match Completable and 2x 1.x: Add Completable.doOnCompleted and deprecate Completable.doOnComplete Feb 14, 2016
@zsxwing
Copy link
Member

zsxwing commented Feb 14, 2016

@zach-klippenstein Thanks! Just updated the title. 👍

akarnokd added a commit that referenced this pull request Feb 14, 2016
1.x: Add Completable.doOnCompleted and deprecate Completable.doOnComplete
@akarnokd akarnokd merged commit ee9956a into ReactiveX:1.x Feb 14, 2016
@stevegury
Copy link
Member

👍

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

Successfully merging this pull request may close these issues.

4 participants