Adding concat and then for values #1029

yilinwei · 2018-01-18T01:49:57Z

Add a few combinators which keeps being used within our codebase.

codecov-io · 2018-01-18T02:00:57Z

Codecov Report

Merging #1029 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1029      +/-   ##
============================================
+ Coverage     83.55%   83.63%   +0.07%     
- Complexity     3420     3433      +13     
============================================
  Files           331      331              
  Lines         26947    26961      +14     
  Branches       4990     4999       +9     
============================================
+ Hits          22516    22549      +33     
+ Misses         2941     2930      -11     
+ Partials       1490     1482       -8

Impacted Files	Coverage Δ	Complexity Δ
...ore/src/main/java/reactor/core/publisher/Mono.java	`95.89% <ø> (-0.76%)`	`240 <0> (+3)`
...ore/src/main/java/reactor/core/publisher/Flux.java	`99.59% <100%> (-0.41%)`	`483 <1> (+3)`
...va/reactor/core/publisher/ParallelMergeReduce.java	`68.59% <0%> (-3.31%)`	`3% <0%> (ø)`
...r-core/src/main/java/reactor/core/Disposables.java	`89.63% <0%> (-0.46%)`	`20% <0%> (ø)`
...ava/reactor/core/publisher/EventLoopProcessor.java	`80.2% <0%> (ø)`	`52% <0%> (ø)`	⬇️
...ain/java/reactor/core/publisher/FluxPublishOn.java	`88.25% <0%> (+0.41%)`	`5% <0%> (ø)`	⬇️
...c/main/java/reactor/core/publisher/FluxExpand.java	`94.22% <0%> (+0.44%)`	`3% <0%> (ø)`	⬇️
...main/java/reactor/core/publisher/FluxGenerate.java	`70.37% <0%> (+0.61%)`	`7% <0%> (ø)`	⬇️
... and 8 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 0a3f5fb...55bc530. Read the comment docs.

akarnokd · 2018-01-18T08:59:18Z

reactor-core/src/main/java/reactor/core/publisher/Flux.java

+	 * @since 3.1.2
+	 */
+	@SafeVarargs
+	public static <T> Flux<T> concat(T... values) {


I don't get why, that's what just(T...) is for.

what @akarnokd said, @yilinwei note the concat isn't necessary and then note the implementation becomes exactly the same as just(T...):

@SafeVarargs public static <T> Flux<T> just(T... data) { return fromArray(data); }

@akarnokd @simonbasle That's because I'm an idiot attempting to commit at 1.30am...

simonbasle · 2018-01-18T11:07:20Z

reactor-core/src/main/java/reactor/core/publisher/Flux.java

+	 * @since 3.1.2
+	 */
+	@SafeVarargs
+	public final Flux<T> concatWith(T... values) {


that one is interesting. I wonder if naming it endWith(T...) would make sense, as a dual to startWith(T...)

simonbasle · 2018-01-18T11:10:20Z

So far we've typically restrained from adding aliases like these when they are simple enough, as we try to limit the number of operators the API offers...

That said, this isn't the first time at least the Mono#then(V) one is suggested. So we might consider it. The concatWith(T...) (maybe renamed as endWith(T...)) has also potential.

any thought @smaldini ?

yilinwei · 2018-01-18T13:55:50Z

@simonbasle

I think in general it's good to limit the number of operations; but I'm curious as to why the policy is to exclude more simple ones?

I can understand rejecting more niche operators, but since there's no easy way to add extension methods in java wouldn't it be better to include the simpler combinators especially those which are unambiguous and likely to be duped across most users codebases?

simonbasle · 2018-01-18T14:01:35Z

@yilinwei simply a mater of favoring the aliases that are a bit more involved to write, since the simple aliases don't remove as much boilerplate.

simonbasle · 2018-01-23T13:13:08Z

@yilinwei discussed it with @smaldini, not sure yet about adding these at all, but if we do we'd prefer the names thenReturn(T...) and concatWithValues(T...). Also can you please remove the misleading @since tags?

simonbasle · 2018-01-25T15:45:29Z

reactor-core/src/test/java/reactor/core/publisher/FluxConcatWithTest.java

@@ -99,4 +99,15 @@ public void dontBreakFluxArrayConcatMap() {
 		.assertNoError()
 		.assertComplete();
 	}
+
+	@Test
+    public void endWith() {


nitpick: rename to concatWithValues and prefer using StepVerifier (rest of the test suite is old and didn't switch to this pattern, but newer tests should prefer StepVerifier)

simonbasle · 2018-01-26T19:20:33Z

reactor-core/src/test/java/reactor/core/publisher/FluxConcatWithTest.java

+    public void concatWithValues() {
+      StepVerifier.create(Flux.just(1, 2).concatWithValues(4, 5, 6))
+          .expectNext(1, 2, 3, 4, 5, 6)
+          .expectComplete();


oops, this should be verifyComplete() or expectComplete().verify()

simonbasle · 2018-01-26T19:26:21Z

reactor-core/src/test/java/reactor/core/publisher/MonoThenIgnoreTest.java

+    public void thenReturn() {
+	    StepVerifier.create(Mono.just(0).thenReturn(2))
+                    .expectNext(2)
+                    .expectComplete();


same here, verifyComplete(). This actually made me notice that a couple of tests wrongly use expectComplete() without a verify() :(

(I'll fix these other pre-existing bad tests on master)

yilinwei · 2018-01-29T13:13:47Z

@simonbasle

Test failure on CI doesn't seem related.

reactor.core.publisher.FluxPublishOnTest > rejectedExecutionExceptionOnErrorSignalExecutorService FAILED
    org.junit.runners.model.TestTimedOutException: test timed out after 5000 milliseconds
        at sun.misc.Unsafe.park(Native Method)
        at java.util.concurrent.locks.LockSupport.park(LockSupport.java:149)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:839)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedInterruptibly(AbstractQueuedSynchronizer.java:992)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly(AbstractQueuedSynchronizer.java:1299)
        at java.util.concurrent.CountDownLatch.await(CountDownLatch.java:203)
        at reactor.core.publisher.FluxPublishOnTest.rejectedExecutionExceptionOnErrorSignalExecutorService(FluxPublishOnTest.java:1140)

simonbasle · 2018-01-29T13:14:57Z

@yilinwei right, I'll restart the job in case this is a transient failure

yilinwei · 2018-01-29T13:37:18Z

Seems like there's more than one possible transient failure.

reactor.core.publisher.FluxBufferWhenTest > bufferedCanCompleteIfOpenNeverCompletesOverlapping FAILED
    java.lang.AssertionError: expectation "expectNext(200)" failed (expected value: 200; actual value: 199)

Adding concat and then for values

aad47e7

akarnokd reviewed Jan 18, 2018

View reviewed changes

Commit tests for concatWith and then.

295d502

simonbasle reviewed Jan 18, 2018

View reviewed changes

Rename concatWith for values to endWith

c641196

simonbasle added this to the 3.1.4.RELEASE milestone Jan 18, 2018

simonbasle added a commit that referenced this pull request Jan 19, 2018

fix #1029 Add a log() overload that directly takes a Logger

46452c1

Rename as per comments.

4cd9a76

simonbasle requested changes Jan 25, 2018

View reviewed changes

Changing name of endWith test and use the StepVerifier

00b501e

simonbasle reviewed Jan 26, 2018

View reviewed changes

simonbasle requested a review from smaldini January 26, 2018 19:25

simonbasle reviewed Jan 26, 2018

View reviewed changes

Fixed tests.

55bc530

simonbasle approved these changes Jan 29, 2018

View reviewed changes

simonbasle mentioned this pull request Jan 29, 2018

Give FluxBufferWhen tests more wiggle room with 5ms -> 25ms interval #1044

Merged

smaldini approved these changes Jan 29, 2018

View reviewed changes

simonbasle added the type/enhancement A general enhancement label Jan 30, 2018

simonbasle merged commit c66fdd8 into reactor:master Jan 30, 2018

simonbasle added a commit that referenced this pull request Jan 30, 2018

reference concatWith(T...) and thenReturn(T) from #1029 in refdoc

0e7af09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding concat and then for values #1029

Adding concat and then for values #1029

yilinwei commented Jan 18, 2018

codecov-io commented Jan 18, 2018 •

edited

Loading

akarnokd Jan 18, 2018

simonbasle Jan 18, 2018

yilinwei Jan 18, 2018

simonbasle Jan 18, 2018

simonbasle commented Jan 18, 2018

yilinwei commented Jan 18, 2018

simonbasle commented Jan 18, 2018

simonbasle commented Jan 23, 2018

simonbasle Jan 25, 2018

simonbasle Jan 26, 2018

simonbasle Jan 26, 2018

simonbasle Jan 26, 2018

yilinwei commented Jan 29, 2018

simonbasle commented Jan 29, 2018

yilinwei commented Jan 29, 2018

Adding concat and then for values #1029

Adding concat and then for values #1029

Conversation

yilinwei commented Jan 18, 2018

codecov-io commented Jan 18, 2018 • edited Loading

Codecov Report

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

simonbasle commented Jan 18, 2018

yilinwei commented Jan 18, 2018

simonbasle commented Jan 18, 2018

simonbasle commented Jan 23, 2018

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

yilinwei commented Jan 29, 2018

simonbasle commented Jan 29, 2018

yilinwei commented Jan 29, 2018

codecov-io commented Jan 18, 2018 •

edited

Loading