2.x: add more Maybe operators, fix a few javadoc mistakes #4467
Conversation
Current coverage is 76.00% (diff: 83.80%)@@ 2.x #4467 diff @@
==========================================
Files 485 493 +8
Lines 33042 33546 +504
Methods 0 0
Messages 0 0
Branches 5240 5310 +70
==========================================
+ Hits 25079 25496 +417
- Misses 5920 5983 +63
- Partials 2043 2067 +24
|
| @@ -231,7 +231,7 @@ public static int bufferSize() { | |||
| public static <T, R> Flowable<R> combineLatest(Publisher<? extends T>[] sources, Function<Object[], ? extends R> combiner, int bufferSize) { | |||
| ObjectHelper.requireNonNull(sources, "sources is null"); | |||
| ObjectHelper.requireNonNull(combiner, "combiner is null"); | |||
| verifyPositive(bufferSize, "bufferSize"); | |||
| ObjectHelper.verifyPositive(bufferSize, "bufferSize"); | |||
|
👍 |
| * @param onEvent the callback to call with the terminal event tuple | ||
| * @return the new Maybe instance | ||
| */ | ||
| public final Maybe<T> doOnEvent(BiConsumer<? super T, ? super Throwable> onEvent) { |
There was a problem hiding this comment.
I'm not sure this is any more ergonomic than doOnEach with a Notification. Will Single and Completable be getting doOnEach or this doOnEvent method?
There was a problem hiding this comment.
Wasn't @vanniktech doing something like this for the others?
There was a problem hiding this comment.
It feels weird to use null as such a strong signal where it's forbidden in most (all?) of the API. Not only do I prefer the readability that Notification's methods provide, but it would allow the re-use of side-effecting operations (such as logging) across all stream types since they all would have the same signature.
There was a problem hiding this comment.
This is allocation free right now, Notification.createOnNext allocates. You can have a utility function that turns a Consumer<Notification<T>> into this BiConsumer but you can't un-allocate a Notification the other way around.
There was a problem hiding this comment.
But by that argument we should remove doOnEach from Observable and Flowable as well in favor of a method with this signature. At least then there'd be parity for peeking across all stream types.
I would much prefer a doOnEach in all the stream types and force those who really need zero allocation peeking to just do a lift or use the doOnEach overload which accepts individual Consumers.
There was a problem hiding this comment.
I'm doing coverage improvement right now on Flowable and Observable but after it, I encourage you to post a PR with your suggestions.
There was a problem hiding this comment.
Yup I'll also add those methods to RxJava 2 on Monday
verifyPositiveintoObjectHelperFlowable.subscribe()andObservable.subscribe()mentioningSubscriptionfrom 1.xMaybe: addamb,concat,concatArray,merge,mergeArray,concatMap,subscribe,subscribeWith,doOnEvent