-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2.x: Add materialize() and dematerialize() #6278
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #6278 +/- ##
============================================
- Coverage 98.25% 98.22% -0.03%
- Complexity 6259 6277 +18
============================================
Files 667 672 +5
Lines 44887 44954 +67
Branches 6213 6216 +3
============================================
+ Hits 44104 44158 +54
- Misses 247 258 +11
- Partials 536 538 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to materialize
Mixed feelings about dematerialize
@CheckReturnValue | ||
@SchedulerSupport(SchedulerSupport.NONE) | ||
@Experimental | ||
public final <T2> Maybe<T2> dematerialize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this API very awkward since it's available on every type but should only be used on Single<Notification<T>>
. I don't have a better suggestion though. Except defining it yourself and using a Kotlin extension function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been the workaround for Observable.dematerialize
too.
The alternative is to have the user select the notification:
Maybe<R> dematerialize(Function<T, Notification<R>> selector);
thus if you have a Single<Notification<T>>
, you only need an identity selector:
Single<Notification<T>> source = ...
Maybe<T> source.dematerialize(v -> v);
However, this selector pattern is not an established one in RxJava right now. We could go for this but then the other existing dematerialize
operators should also get a selector overload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know about dematerialize
, then I guess we stick to that convention. Providing a mapper sounds good. Then maybe we could deprecate the current dematerialize
in Observable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay then. I'll change this to use the selector and post a separate PR for adding a selector variant to Observable
/Flowable
and deprecate the type-fragile versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
import io.reactivex.subjects.SingleSubject; | ||
|
||
public class SingleDematerializeTest { | ||
|
||
@Test | ||
public void success() { | ||
Single.just(Notification.createOnNext(1)) | ||
.<Integer>dematerialize() | ||
.dematerialize(Functions.<Notification<Integer>>identity()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too bad the identity function is inside an internal package :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose most users now can use lambdas in their project so this is just v -> v
for them. Only we are still so unlucky to use such helper methods.
This PR adds the
materialize
operator toMaybe
,Single
andCompletable
to turn their signals into the correspondingNotification
object. This operator has been available forObservable
s (andFlowable
s) from the beginning of the Rx API. The methods returnSingle<Notification<T>>
.To complement, the
dematerialize
operator is only defined forSingle
and results in aMaybe
.If accepted, I'll draw the correct marble diagrams for them in a separate PR.
Resolves: #6272