implement Maybe.switchIfEmpty(Single) by bmaslakov · Pull Request #5582 · ReactiveX/RxJava

bmaslakov · 2017-09-02T18:16:33Z

Adds Maybe.switchIfEmpty(Single), fixes #4544.

codecov · 2017-09-02T18:29:48Z

Codecov Report

Merging #5582 into 2.x will decrease coverage by 0.02%.
The diff coverage is 97.36%.

@@             Coverage Diff              @@
##                2.x    #5582      +/-   ##
============================================
- Coverage     96.15%   96.13%   -0.03%     
- Complexity     5821     5825       +4     
============================================
  Files           631      632       +1     
  Lines         41421    41459      +38     
  Branches       5739     5742       +3     
============================================
+ Hits          39830    39858      +28     
- Misses          630      645      +15     
+ Partials        961      956       -5

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/reactivex/Maybe.java	`100% <100%> (ø)`	`169 <1> (+1)`	⬆️
...rnal/operators/maybe/MaybeSwitchIfEmptySingle.java	`97.22% <97.22%> (ø)`	`2 <2> (?)`
...nternal/operators/observable/ObservableCreate.java	`79.48% <0%> (-18.81%)`	`2% <0%> (ø)`
...in/java/io/reactivex/subjects/BehaviorSubject.java	`86.97% <0%> (-5.73%)`	`57% <0%> (ø)`
.../io/reactivex/internal/functions/ObjectHelper.java	`94.73% <0%> (-5.27%)`	`20% <0%> (-1%)`
...rnal/operators/flowable/FlowableFlatMapSingle.java	`93.47% <0%> (-3.27%)`	`2% <0%> (ø)`
...a/io/reactivex/internal/util/QueueDrainHelper.java	`58.86% <0%> (-2.84%)`	`31% <0%> (-2%)`
...tivex/internal/schedulers/TrampolineScheduler.java	`92.3% <0%> (-2.57%)`	`6% <0%> (ø)`
...a/io/reactivex/internal/queue/MpscLinkedQueue.java	`98.03% <0%> (-1.97%)`	`17% <0%> (-1%)`
...rnal/subscriptions/DeferredScalarSubscription.java	`98.46% <0%> (-1.54%)`	`28% <0%> (-1%)`
... and 27 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 14bebc5...4dadbb9. Read the comment docs.

akarnokd · 2017-09-02T18:31:15Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeSwitchIfEmptyToMaybeTest.java

        Maybe.<Integer>empty().switchIfEmpty(Maybe.just(2)).test().assertResult(2);
    }

-    @Test


Why were these removed?

akarnokd · 2017-09-02T18:34:22Z

src/main/java/io/reactivex/Maybe.java

+     */
+    @CheckReturnValue
+    @SchedulerSupport(SchedulerSupport.NONE)
+    public final Maybe<T> switchIfEmpty(SingleSource<? extends T> other) {


The issue mentioned Single as return type.

akarnokd · 2017-09-02T18:35:43Z

src/main/java/io/reactivex/Maybe.java

+     *              the alternate SingleSource to subscribe to if the main does not emit any items
+     * @return  a Maybe that emits the items emitted by the source Maybe or the item of an
+     *          alternate SingleSource if the source Maybe is empty.
+     */


Please add the appropriate experimental tags and annotations:

@since 2.1.4 - experimental

akarnokd · 2017-09-02T18:36:07Z

src/main/java/io/reactivex/Maybe.java

+     *          alternate SingleSource if the source Maybe is empty.
+     */
+    @CheckReturnValue
+    @SchedulerSupport(SchedulerSupport.NONE)


Please add @Experimental

akarnokd · 2017-09-02T18:37:17Z

src/main/java/io/reactivex/internal/operators/maybe/MaybeSwitchIfEmptyToMaybe.java

 * @param <T> the value type
 */
-public final class MaybeSwitchIfEmpty<T> extends AbstractMaybeWithUpstream<T, T> {
+public final class MaybeSwitchIfEmptyToMaybe<T> extends AbstractMaybeWithUpstream<T, T> {


I'd keep the original name, less change per PR, and name the new version MaybeSwitchIfEmptySingle.

akarnokd · 2017-09-02T18:38:13Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeSwitchIfEmptyToMaybeTest.java

 import io.reactivex.schedulers.Schedulers;

-public class MaybeSwitchIfEmptyTest {
+public class MaybeSwitchIfEmptyToMaybeTest {


Same as before, please keep the original name to this test.

akarnokd · 2017-09-02T18:39:11Z

src/main/java/io/reactivex/Maybe.java

    public final Maybe<T> defaultIfEmpty(T defaultItem) {
        ObjectHelper.requireNonNull(defaultItem, "item is null");
-        return switchIfEmpty(just(defaultItem));
+        return switchIfEmpty(Single.just(defaultItem));


Why was this changed, the original just should still work?!

Of course the original just still works. Just thought that it would be better to wrap the T defaultItem into Single instead of Maybe because the defaultItem cannot be empty.

Please restore the original state. The less change to existing and practically unrelated code regarding the PR the better.

akarnokd · 2017-09-02T18:43:17Z

I'm only 50% convinced for the need of this overload.

… the PR

reevaluating

akarnokd · 2017-09-02T19:57:56Z

/cc @vanniktech @artem-zinnatullin

artem-zinnatullin

LGTM, few comments

artem-zinnatullin · 2017-09-03T00:07:48Z

src/main/java/io/reactivex/internal/operators/maybe/MaybeSwitchIfEmptySingle.java

+
+            final SingleObserver<? super T> actual;
+
+            final AtomicReference<Disposable> parent;


OtherSingleObserver can extend AtomicReference to save field, should we do that @akarnokd?

If you look at L95, you'll see that parent is SwitchIfEmptyMaybeObserver which is declared as AtomicReference<Disposable> thus this is not a new instace of an AtomicReference here.

Yes, I saw that, but wouldn't it save field anyway? Though it's super nit

The downstream can interact with only one replaceable reference to the original or the alternative disposable, so you'll need an extra field either way.

artem-zinnatullin · 2017-09-03T00:09:32Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeSwitchIfEmptySingleTest.java

+
+    @Test
+    public void nonEmpty() {
+        Maybe.just(1).switchIfEmpty(Single.just(2)).test().assertResult(1);


assertValuesOnly() @vanniktech?

onSuccess translates to onNext + onComplete in TestObserver so this requires assertResult.

AssertValuesOnly is mostly for Observable & Flowable

artem-zinnatullin · 2017-09-03T00:10:29Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeSwitchIfEmptySingleTest.java

+    @Test
+    public void error() {
+        Maybe.<Integer>error(new TestException()).switchIfEmpty(Single.just(2))
+        .test().assertFailure(TestException.class);


Would be great to also check that no values were emitted to check that it didn't switch to fallback Single

assertFailure does check values:

public final U assertFailure(Class<? extends Throwable> error, T... values) { return assertSubscribed() .assertValues(values) .assertError(error) .assertNotComplete(); }

Yes assertFailure does the job under the hood

vanniktech

I like it

implement Maybe.switchIfEmpty(Single)

be1bfe6

akarnokd previously requested changes Sep 2, 2017

View reviewed changes

bmaslakov added 2 commits September 2, 2017 22:34

switchIfEmpty(Single) returns single; remove all changes unrelated to…

31d2d81

… the PR

add 'experimental' annotation

4dadbb9

artem-zinnatullin reviewed Sep 3, 2017

View reviewed changes

akarnokd added 2.x Feature-Request labels Sep 4, 2017

vanniktech approved these changes Sep 4, 2017

View reviewed changes

akarnokd merged commit c43229b into ReactiveX:2.x Sep 4, 2017

akarnokd mentioned this pull request Sep 18, 2017

2.1.4 release preparation #5607

Closed

jared-bot mentioned this pull request Sep 27, 2017

Dependency update: rxjava xmartlabs/bigbang-template#14

Merged


		final SingleObserver<? super T> actual;

		final AtomicReference<Disposable> parent;

Conversation

bmaslakov commented Sep 2, 2017

Uh oh!

codecov bot commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmaslakov Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akarnokd commented Sep 2, 2017

Uh oh!

akarnokd commented Sep 2, 2017

Uh oh!

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanniktech left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 2, 2017 •

edited

Loading

bmaslakov Sep 2, 2017 •

edited

Loading