see #264 Add Flux.doOnEach with Signal and individual consumers #288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Current coverage is 60.71% (diff: 100%)@@ master #288 diff @@
==========================================
Files 252 252
Lines 23207 24611 +1404
Methods 0 0
Messages 0 0
Branches 4235 4534 +299
==========================================
+ Hits 13986 14942 +956
- Misses 7425 7800 +375
- Partials 1796 1869 +73
|
dfeist
reviewed
Dec 7, 2016
| */ | ||
| public final Flux<T> doOnEach(Consumer<? super T> nextConsumer, | ||
| Consumer<? super Throwable> errorConsumer, | ||
| Runnable completeRunnable) { |
There was a problem hiding this comment.
IMO:
- This isn't 100% required, using consecutive doOnNext() and doOnError() isn't particularly much more verbose.
- If this method does exist i would either, limit it to next/error consumer only, or expand it to include afterTerminate runnable too so it has all signals.
dfeist
reviewed
Dec 7, 2016
| } | ||
|
|
||
| /** | ||
| * Triggers side-effects when the {@link Flux} is subscribed, emits an item, fails |
There was a problem hiding this comment.
This is really only for the advanced use cases. So if it is exposed, which I don't think is a bad idea, I would explicitly state in Javadoc that it is for advanced used cases and that generally the other more specific doOnXX should be used instead, else we risk people using this by accident and over-complicating things.
|
Looks good. |
smaldini
approved these changes
Dec 8, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not the original ask from #264 but these
doOnEachvariants have value.