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

2.x: Add missing Resource Observer for Maybe, Completable & Single and adjust some Javadoc #4518

Merged
merged 3 commits into from
Sep 9, 2016
Merged

2.x: Add missing Resource Observer for Maybe, Completable & Single and adjust some Javadoc #4518

merged 3 commits into from
Sep 9, 2016

Conversation

vanniktech
Copy link
Collaborator

Fixes #4517

@akarnokd akarnokd added this to the 2.0 RC 3 milestone Sep 9, 2016
@@ -62,10 +65,6 @@ public void onComplete() {

dispose();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for coverage on the protected cancel method.

@codecov-io
Copy link

codecov-io commented Sep 9, 2016

Current coverage is 78.60% (diff: 100%)

Merging #4518 into 2.x will increase coverage by 0.07%

@@                2.x      #4518   diff @@
==========================================
  Files           507        510     +3   
  Lines         34386      34424    +38   
  Methods           0          0          
  Messages          0          0          
  Branches       5395       5401     +6   
==========================================
+ Hits          27003      27059    +56   
+ Misses         5411       5401    -10   
+ Partials       1972       1964     -8   

Powered by Codecov. Last update 4f878d5...120d760

@akarnokd
Copy link
Member

akarnokd commented Sep 9, 2016

It might be worth considering removing the protected cancel() methods as they are duplicates of dispose. For ResourceSubscriber I don't know because it works with Subscription.cancel() but also is a Disposable so dispose() is there as well.

@vanniktech
Copy link
Collaborator Author

So should I remove the cancel() method on all Resource*Observer?

@akarnokd
Copy link
Member

akarnokd commented Sep 9, 2016

@JakeWharton your opinion on the removal?

@JakeWharton
Copy link
Member

I'm supportive of removing cancel(). I much prefer when there's only one way to do something!

@akarnokd
Copy link
Member

akarnokd commented Sep 9, 2016

👍

@akarnokd akarnokd merged commit 63c4451 into ReactiveX:2.x Sep 9, 2016
@vanniktech vanniktech deleted the reactive_resource_observers branch September 9, 2016 15:54
@akarnokd akarnokd mentioned this pull request Sep 21, 2016
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.

None yet

4 participants