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: rename emitters #isCancelled to #isDisposed #4490

Merged
merged 1 commit into from
Sep 6, 2016

Conversation

Mauin
Copy link
Contributor

@Mauin Mauin commented Sep 6, 2016

Renames Observable/Single/CompletableEmitter#isCancelled to #isDisposed.

This now causes a bit of a weird situation where most (not all) Emitter classes implement Disposable as well, so both interfaces expose the same #isDisposed method. Before the #isCancelled methods were basically just calling #isDisposed or had the same implementation.

Suggestions on how to improve this?

@codecov-io
Copy link

Current coverage is 77.91% (diff: 0.00%)

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

@@                2.x      #4490   diff @@
==========================================
  Files           500        500          
  Lines         33909      33905     -4   
  Methods           0          0          
  Messages          0          0          
  Branches       5325       5325          
==========================================
+ Hits          26392      26416    +24   
+ Misses         5547       5517    -30   
- Partials       1970       1972     +2   

Powered by Codecov. Last update ff3c5d0...80ebe35

@JakeWharton
Copy link
Member

I'm tempted to remove setDisposable from the emitter and keep this method named isCanceled(). If you want to set a disposable why not use unsafeCreate and ObservableSource directly?

@akarnokd
Copy link
Member

akarnokd commented Sep 6, 2016

@JakeWharton these are the safe versions that also manages a resource for you without the need to worry about (and lose) backpressure or cancellation in general from unsafeCreate.

@akarnokd akarnokd added this to the 2.0 RC 3 milestone Sep 6, 2016
@akarnokd
Copy link
Member

akarnokd commented Sep 6, 2016

Otherwise 👍

@JakeWharton
Copy link
Member

I'm not sure how that is an argument against what I said. Having both setCancelable and setDisposable in the API is ugly and redundant.

@akarnokd
Copy link
Member

akarnokd commented Sep 6, 2016

If you believe users don't really need the Disposable overload then I'm open to remove them. Also consider dropping Disposables.from variants not in use and adding Cancellables.from.

@Mauin
Copy link
Contributor Author

Mauin commented Sep 6, 2016

As for setCancelable and setDisposable I agree with @JakeWharton. Would probably be best to somehow merge them or get rid of one to clean the API. But that would be a separate issue/PR, I guess?

@akarnokd akarnokd merged commit 10f727f into ReactiveX:2.x Sep 6, 2016
@akarnokd
Copy link
Member

akarnokd commented Sep 6, 2016

Yes, let's have a separate PR for that.

@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants