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: more detailed no-multi-subscribe with std consumers error message #5301

Merged
merged 2 commits into from Apr 20, 2017

Conversation

akarnokd
Copy link
Member

This PR changes the "Disposable already set!" and "Subscription already set!" messages on the standard consumer classes (DisposableSubscriber, DisposableObserver, etc.) to something more meaningful:

"It is not allowed to subscribe with a(n) <class name> multiple times. Please create a fresh instance of <class name> and subscribe that to the target source instead."

Where <class name> is a placeholder for the getClass().getName() of the subclass of those consumer types. It should clearly state to avoid subscribing with them multiple times as well as printing the full class name to indicate the problem is with the use of the implementor class, and not with the abstract RxJava class.

Inspired by this StackOverflow question, one of many such questions.

For the internal operators, the original error message stays because when they appear, that is still likely due to an implementation bug (or a misbehaving user-created custom implementation).

@akarnokd akarnokd added this to the 2.0 backlog milestone Apr 20, 2017
@codecov
Copy link

codecov bot commented Apr 20, 2017

Codecov Report

Merging #5301 into 2.x will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5301      +/-   ##
============================================
- Coverage     96.17%   96.09%   -0.09%     
- Complexity     5756     5774      +18     
============================================
  Files           628      630       +2     
  Lines         41085    41148      +63     
  Branches       5703     5714      +11     
============================================
+ Hits          39514    39540      +26     
- Misses          613      638      +25     
- Partials        958      970      +12
Impacted Files Coverage Δ Complexity Δ
...n/java/io/reactivex/observers/DefaultObserver.java 100% <100%> (+10%) 5 <2> (+1) ⬆️
...va/io/reactivex/subscribers/DefaultSubscriber.java 100% <100%> (+13.33%) 7 <2> (+2) ⬆️
.../io/reactivex/observers/ResourceMaybeObserver.java 100% <100%> (ø) 8 <2> (ø) ⬇️
...tivex/observers/DisposableCompletableObserver.java 100% <100%> (ø) 7 <2> (ø) ⬇️
...activex/observers/ResourceCompletableObserver.java 100% <100%> (ø) 8 <2> (ø) ⬇️
.../reactivex/observers/DisposableSingleObserver.java 100% <100%> (ø) 7 <2> (ø) ⬇️
...a/io/reactivex/subscribers/ResourceSubscriber.java 100% <100%> (ø) 10 <3> (+1) ⬆️
...o/reactivex/observers/DisposableMaybeObserver.java 100% <100%> (ø) 7 <2> (ø) ⬇️
.../io/reactivex/internal/util/EndConsumerHelper.java 100% <100%> (ø) 15 <15> (?)
.../java/io/reactivex/observers/ResourceObserver.java 100% <100%> (ø) 8 <2> (ø) ⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db62772...798c9bd. Read the comment docs.

Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Reviewed on mobile. I tried my best.

@akarnokd
Copy link
Member Author

Thanks Jake. We can refine the message later if necessary.

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

2 participants