2.x: add Observable.switchMapSingle and switchMapSingleDelayError by davidmoten · Pull Request #5161 · ReactiveX/RxJava

davidmoten · 2017-03-08T09:48:32Z

This is a new operator discussed in #4853. The issue refers to a goodly number of new operators which I'll do bit by bit as my time allows and to ensure review is not too daunting.

akarnokd

Looks good but some changes are required.

akarnokd · 2017-03-08T10:00:36Z

src/main/java/io/reactivex/internal/operators/observable/ObservableInternalHelper.java

+                } else {
+                    single = Single.unsafeCreate(source);
+                }
+               return (Observable<R>) single.toObservable();


You could just use:

return RxJavaPlugins.onAssembly(new SingleToObservable(mapper.apply(t)));

akarnokd · 2017-03-08T10:02:50Z

src/main/java/io/reactivex/Observable.java

+     * @since 2.0.8
+     */
+    @Experimental
+    @CheckReturnValue


@NonNull?

akarnokd · 2017-03-08T10:03:03Z

src/main/java/io/reactivex/Observable.java

+     * @since 2.0.8
+     */
+    @Experimental
+    @CheckReturnValue


@NonNull Function?

akarnokd · 2017-03-08T10:03:48Z

src/main/java/io/reactivex/internal/operators/observable/ObservableInternalHelper.java

+
+    private static <T, R> Function<T, Observable<R>> convertSingleMapperToObservableMapper(
+            final Function<? super T, ? extends SingleSource<? extends R>> mapper) {
+        return new Function<T, Observable<R>>() {


ObjectHelper.requireNonNull(mapper, "mapper is null");

Please avoid anonymous inner classes.

davidmoten · 2017-03-08T10:49:49Z

updated with suggested changes

davidmoten · 2017-03-08T11:12:52Z

unrelated ci failure

io.reactivex.parallel.ParallelFlowableTest > parallelismAndPrefetchAsync FAILED
    java.lang.AssertionError: Value counts differ; Expected: 1048576, Actual: 814189 (latch = 1, values = 814189, errors = 0, completions = 0, timeout!, disposed!)
        at io.reactivex.observers.BaseTestConsumer.fail(BaseTestConsumer.java:163)
        at io.reactivex.observers.BaseTestConsumer.assertValueCount(BaseTestConsumer.java:462)
        at io.reactivex.parallel.ParallelFlowableTest.parallelismAndPrefetchAsync(ParallelFlowableTest.java:741)

akarnokd · 2017-03-08T11:15:14Z

unrelated ci failure

Tracking via #5154. Could be due to low timeout settings and Travis overload. Just rerun the build next time; I did it just now.

vanniktech · 2017-03-08T11:18:44Z

src/test/java/io/reactivex/internal/operators/observable/ObservableSwitchTest.java

+    @Test
+    public void switchMapSingleFunctionDoesntReturnSingle() {
+        Observable.just(0)
+        .switchMapSingle(new Function<Object, SingleSource<Integer>>() {


I remember that with flatMapSingle there was in the initial draft a few errors that errors weren't propagated correctly. Should not those cases also be tested that in the future once the operators receive custom implementations everything still works correctly?

I don't think so. At the moment those cases have full coverage. If we make custom implementations then a whole suite of tests need to be added just get coverage back up again and it will be an obvious timesaver to duplicate the tests you refer to.

vanniktech · 2017-03-08T11:19:06Z

src/main/java/io/reactivex/internal/operators/observable/ObservableInternalHelper.java

+
+        @Override
+        public Observable<R> apply(T t) throws Exception {
+            return RxJavaPlugins.onAssembly(new SingleToObservable<R>(mapper.apply(t)));


check that mapper.apply(t) does not return null?

Indeed, this should do it:

return RxJavaPlugins.onAssembly(new SingleToObservable<R>( ObjectHelper.requireNonNull(mapper.apply(t), "The mapper returned a null value")));

thanks will do

codecov · 2017-03-08T11:28:05Z

Codecov Report

Merging #5161 into 2.x will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #5161      +/-   ##
============================================
+ Coverage     95.86%   95.88%   +0.02%     
- Complexity     5648     5652       +4     
============================================
  Files           621      621              
  Lines         39962    39972      +10     
  Branches       5610     5610              
============================================
+ Hits          38309    38329      +20     
+ Misses          665      657       -8     
+ Partials        988      986       -2

Impacted Files	Coverage Δ	Complexity Δ
...operators/observable/ObservableInternalHelper.java	`86.31% <100%> (+1.25%)`	`18 <3> (+3)`	✅
src/main/java/io/reactivex/Observable.java	`100% <100%> (ø)`	`506 <2> (+2)`	✅
...vex/internal/operators/single/SingleTakeUntil.java	`86.88% <0%> (-8.2%)`	`2% <0%> (ø)`
.../operators/completable/CompletableConcatArray.java	`93.75% <0%> (-6.25%)`	`2% <0%> (ø)`
...rnal/subscriptions/DeferredScalarSubscription.java	`93.84% <0%> (-4.62%)`	`27% <0%> (-1%)`
...io/reactivex/internal/util/BackpressureHelper.java	`95.91% <0%> (-4.09%)`	`21% <0%> (-1%)`
...ernal/operators/maybe/MaybeTakeUntilPublisher.java	`96% <0%> (-4%)`	`2% <0%> (ø)`
...main/java/io/reactivex/subjects/SingleSubject.java	`95.23% <0%> (-2.39%)`	`38% <0%> (-1%)`
...ternal/operators/completable/CompletableUsing.java	`95.23% <0%> (-2.39%)`	`4% <0%> (ø)`
.../main/java/io/reactivex/subjects/MaybeSubject.java	`95.65% <0%> (-2.18%)`	`46% <0%> (-1%)`
... and 35 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 7861679...315fb80. Read the comment docs.

davidmoten · 2017-03-08T20:52:05Z

updated with null check on mapper call and added unit tests for null mapper and null mapper call result. Not proposing to duplicate all tests. @akarnokd what would you like?

akarnokd · 2017-03-08T21:05:35Z

Don't duplicate tests. When the specialized implementation happens, that will ask for proper coverage by itself.

akarnokd added 2.x Enhancement labels Mar 8, 2017

akarnokd added this to the 2.1 milestone Mar 8, 2017

akarnokd reviewed Mar 8, 2017

View reviewed changes

davidmoten force-pushed the switchmap-single branch from f2e0b0e to b733386 Compare March 8, 2017 10:47

vanniktech reviewed Mar 8, 2017

View reviewed changes

add Observable.switchMapSingle

315fb80

davidmoten force-pushed the switchmap-single branch from b733386 to 315fb80 Compare March 8, 2017 20:49

akarnokd merged commit b5501c5 into ReactiveX:2.x Mar 9, 2017

akarnokd mentioned this pull request Mar 24, 2017

2.0.8 release preparations #5220

Closed

michaelEllisUy mentioned this pull request Jun 1, 2017

Dependency update: rxjava xmartlabs/bigbang#82

Closed

akarnokd mentioned this pull request Nov 1, 2017

2.x: switchMapSingle and switchMapCompletable #4853

Closed

Conversation

davidmoten commented Mar 8, 2017

Uh oh!

akarnokd 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!

davidmoten commented Mar 8, 2017

Uh oh!

davidmoten commented Mar 8, 2017

Uh oh!

akarnokd commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

Uh oh!

Codecov Report

Uh oh!

davidmoten commented Mar 8, 2017

Uh oh!

akarnokd commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akarnokd commented Mar 8, 2017 •

edited

Loading

codecov bot commented Mar 8, 2017 •

edited

Loading