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: Fix subscribeWith documentation examples #5647

Merged
merged 1 commit into from
Oct 9, 2017

Conversation

dimsuz
Copy link
Contributor

@dimsuz dimsuz commented Oct 5, 2017

This PR improves a documentation of subscribeWith functions in Single, Maybe, Completable by making sure examples are compilable (by using classes which implement Disposable).

This should fix issue #5642 .

In that issue @akarnokd approved my idea to use DisposableMaybeObserver in the example, but as I looked in other classes, I noticed that Observable.subscribeWith() documentation uses a ResourceObserver (which implements Disposable), so I thought that this is the way to go in all other top-classes?

If you think that this is not the case, and instead all of them should be switched to use Disposable*Observer in their examples, let me know, I will do it in this way then.

I think that all those example should use similar classes.

@akarnokd
Copy link
Member

akarnokd commented Oct 5, 2017

ResourceXObserver is generally not necessary so DisposableXObserver should be in the docs.

@akarnokd akarnokd added this to the 2.2 milestone Oct 5, 2017
@codecov
Copy link

codecov bot commented Oct 5, 2017

Codecov Report

Merging #5647 into 2.x will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5647      +/-   ##
============================================
- Coverage     96.19%   96.16%   -0.04%     
- Complexity     5843     5847       +4     
============================================
  Files           634      634              
  Lines         41539    41539              
  Branches       5752     5752              
============================================
- Hits          39960    39945      -15     
- Misses          626      642      +16     
+ Partials        953      952       -1
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/reactivex/Single.java 100% <ø> (ø) 135 <0> (ø) ⬇️
src/main/java/io/reactivex/Maybe.java 100% <ø> (ø) 169 <0> (ø) ⬇️
src/main/java/io/reactivex/Completable.java 100% <ø> (ø) 113 <0> (ø) ⬇️
src/main/java/io/reactivex/Observable.java 100% <ø> (ø) 506 <0> (ø) ⬇️
...nternal/operators/observable/ObservableCreate.java 79.48% <0%> (-15.39%) 2% <0%> (ø)
.../operators/flowable/FlowableBlockingSubscribe.java 91.89% <0%> (-5.41%) 9% <0%> (-1%)
...tivex/internal/observers/FutureSingleObserver.java 94.33% <0%> (-3.78%) 24% <0%> (-1%)
.../internal/operators/flowable/FlowableInterval.java 94.44% <0%> (-2.78%) 3% <0%> (ø)
...vex/internal/operators/flowable/FlowableCache.java 92.61% <0%> (-2.69%) 7% <0%> (ø)
...ernal/operators/flowable/FlowableFromIterable.java 93.04% <0%> (-2.14%) 5% <0%> (ø)
... and 26 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 748fb50...61216ac. Read the comment docs.

@dimsuz
Copy link
Contributor Author

dimsuz commented Oct 6, 2017

Got it, will update soon. Is it ok to force push into PR branches?

@akarnokd
Copy link
Member

akarnokd commented Oct 6, 2017

PR branches are in your own repo so yes.

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.

2 participants