2.x: Add retry(times, predicate) to Single & Completable and verify behavior across them and Maybe. #5753

vanniktech · 2017-12-05T15:03:01Z

Add retry(times, predicate) to Single & Completable (Maybe had it already) which just forwards to Flowable for now
verify behavior across the three reactive types

Happy to get better testing names

…ehavior across them and Maybe.

vanniktech · 2017-12-05T15:09:35Z

Given you've added this to the 2.2 milestone should I update the @since to 2.2.0 then?

akarnokd · 2017-12-05T15:15:17Z

There is going to be a couple of 2.1.x till then. I don't want to release 2.2.0 until the marbles get fixed.

codecov · 2017-12-05T15:37:43Z

Codecov Report

Merging #5753 into 2.x will decrease coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #5753      +/-   ##
============================================
- Coverage     96.32%   96.28%   -0.04%     
+ Complexity     5842     5840       -2     
============================================
  Files           634      634              
  Lines         41650    41653       +3     
  Branches       5769     5769              
============================================
- Hits          40120    40107      -13     
- Misses          600      617      +17     
+ Partials        930      929       -1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/reactivex/Completable.java	`100% <100%> (ø)`	`115 <1> (+1)`	⬆️
src/main/java/io/reactivex/Single.java	`100% <100%> (ø)`	`137 <1> (+1)`	⬆️
...nternal/operators/observable/ObservableCreate.java	`79.48% <0%> (-18.81%)`	`2% <0%> (ø)`
...rnal/operators/flowable/FlowableFlatMapSingle.java	`92.93% <0%> (-3.81%)`	`2% <0%> (ø)`
...tivex/internal/observers/FutureSingleObserver.java	`94.33% <0%> (-3.78%)`	`24% <0%> (-1%)`
.../io/reactivex/internal/schedulers/IoScheduler.java	`89.24% <0%> (-3.23%)`	`9% <0%> (ø)`
...activex/internal/observers/QueueDrainObserver.java	`61.53% <0%> (-2.57%)`	`12% <0%> (-1%)`
.../main/java/io/reactivex/subjects/MaybeSubject.java	`97.82% <0%> (-2.18%)`	`47% <0%> (-1%)`
...nternal/operators/observable/ObservableWindow.java	`98% <0%> (-2%)`	`3% <0%> (ø)`
...ex/internal/subscribers/InnerQueuedSubscriber.java	`96.07% <0%> (-1.97%)`	`18% <0%> (-1%)`
... and 32 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 4d2e821...a89cb4e. Read the comment docs.

artem-zinnatullin

Pls check actual number of retries in tests, otherwise LGTM :)

artem-zinnatullin · 2017-12-07T01:58:09Z

src/main/java/io/reactivex/Completable.java

@@ -1545,6 +1545,28 @@ public final Completable retry(long times) {
        return fromPublisher(toFlowable().retry(times));
    }

+    /**
+     * Returns a Completable that when this Completable emits an error, retries at most times


nit: Phrasing seems a little bit off here, something like "Returns a Completable that retries when …" sounds more natural I guess

I kept this in sync with all of the other retry* java doc sentences.

uh, yeah all rx types have different phrasing for retry operator, alight

artem-zinnatullin · 2017-12-07T02:01:10Z

src/main/java/io/reactivex/Single.java

@@ -2649,6 +2649,26 @@ public final T blockingGet() {
        return toSingle(toFlowable().retry(predicate));
    }

+    /**
+     * Repeatedly re-subscribe at most times or until the predicate returns false, whichever happens first


Let's make it more consistent with javadoc you've added for Completable? It didn't mention Repeatedly and was emphasizing error condition, while with this javadoc you only realize in the end of it that error is initial trigger for whole operator

It should be yeah but if we only change this method then this one is inconsistent with all of the other Single methods.

artem-zinnatullin · 2017-12-07T02:03:31Z

src/test/java/io/reactivex/completable/CompletableRetryTest.java

+                }
+            })
+            .test()
+            .assertFailure(IllegalArgumentException.class);


hm, I believe you should check atomicInteger value here to verify actual number of action retries

Same below and in tests for other reactive types

akarnokd · 2017-12-07T09:26:24Z

The latest changes broke the tests.

artem-zinnatullin

👍

artem-zinnatullin · 2017-12-07T22:09:09Z

src/main/java/io/reactivex/Completable.java

@@ -1545,6 +1545,28 @@ public final Completable retry(long times) {
        return fromPublisher(toFlowable().retry(times));
    }

+    /**
+     * Returns a Completable that when this Completable emits an error, retries at most times


uh, yeah all rx types have different phrasing for retry operator, alight

2.x: Add retry(times, predicate) to Single & Completable and verify b…

6251268

…ehavior across them and Maybe.

akarnokd added 2.x Enhancement labels Dec 5, 2017

akarnokd added this to the 2.2 milestone Dec 5, 2017

akarnokd approved these changes Dec 5, 2017

View reviewed changes

Adjust ParamValidationCheckerTest for the 2 newly added methods.

068f7dd

akarnokd approved these changes Dec 5, 2017

View reviewed changes

artem-zinnatullin suggested changes Dec 7, 2017

View reviewed changes

Add AtomicInteger for checking the number of subscribe calls.

657dad9

Fix copy pasta mistake.

a89cb4e

artem-zinnatullin approved these changes Dec 7, 2017

View reviewed changes

akarnokd merged commit 34242e1 into ReactiveX:2.x Dec 7, 2017

vanniktech deleted the 2.xretryTimesPredicate branch December 8, 2017 07:34

akarnokd mentioned this pull request Dec 18, 2017

2.1.8 release preparations #5769

Closed

jared-bot mentioned this pull request Dec 27, 2017

Dependency update: rxjava xmartlabs/bigbang-template#45

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.x: Add retry(times, predicate) to Single & Completable and verify behavior across them and Maybe. #5753

2.x: Add retry(times, predicate) to Single & Completable and verify behavior across them and Maybe. #5753

vanniktech commented Dec 5, 2017

vanniktech commented Dec 5, 2017 •

edited

Loading

akarnokd commented Dec 5, 2017

codecov bot commented Dec 5, 2017 •

edited

Loading

artem-zinnatullin left a comment

artem-zinnatullin Dec 7, 2017

vanniktech Dec 7, 2017

artem-zinnatullin Dec 7, 2017

artem-zinnatullin Dec 7, 2017

vanniktech Dec 7, 2017

artem-zinnatullin Dec 7, 2017

artem-zinnatullin Dec 7, 2017

akarnokd commented Dec 7, 2017

artem-zinnatullin left a comment

artem-zinnatullin Dec 7, 2017

2.x: Add retry(times, predicate) to Single & Completable and verify behavior across them and Maybe. #5753

2.x: Add retry(times, predicate) to Single & Completable and verify behavior across them and Maybe. #5753

Conversation

vanniktech commented Dec 5, 2017

vanniktech commented Dec 5, 2017 • edited Loading

akarnokd commented Dec 5, 2017

codecov bot commented Dec 5, 2017 • edited Loading

Codecov Report

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akarnokd commented Dec 7, 2017

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanniktech commented Dec 5, 2017 •

edited

Loading

codecov bot commented Dec 5, 2017 •

edited

Loading