Add takeUntil support in Single #3712
Conversation
|
Just noticed my IDE swapped the wildcard imports for explicit ones. Let me know if I should revert that. |
|
Yes please. |
|
|
||
| @Test | ||
| @SuppressWarnings("unchecked") | ||
| public void testTakeUntilSuccess() { |
There was a problem hiding this comment.
No need for test prefix here and in tests below
There was a problem hiding this comment.
Can you please add Single and Observable to all test names appropriately?
Idk, but current tests names are not very obvious, would be easier to see what to expect from tests if they have names like takeUntil_should..When.. but it's personal thing, so feel free to ignore :)
There was a problem hiding this comment.
|
Just style issues, otherwise LGTM 👍 |
|
CC @akarnokd @artem-zinnatullin For the tests, most of them are adapted from Also, if we stick with one style here, should we update the style in |
|
I'm not too keen on how the tests are named or what test framework objects you use. Please squash your commits. |
ZacSweers
force-pushed
the
z/single_takeuntil
branch
from
d961f02 to
e917c77
Compare
|
Squashed |
|
👍 |
|
@zsxwing @stevegury any input? Need another collaborator review Main points I'm wondering about:
|
|
👍 |
This behavior is really confusing. Why not be same as Observable? Am I missing anything? |
|
@akarnokd and I discussed it in #3708. |
|
@hzsweers Thanks for clarifying. However, I would expect |
|
The operators should stay in the same type as long as they can and there are operators that simply can't behave the same as their counterpart in other reactive types. If one wishes the |
If so, I vote for |
|
That's fair, but is there something we could do to at least indicate whether the source was just unsubscribed or actually misbehaved? I was of the impression that Consider the following With PublishSubject<Integer> source = PublishSubject.create();
PublishSubject<Integer> until = PublishSubject.create();
source.take(1).toSingle()
.takeUntil(until.take(1).toSingle())
.subscribe(
new Action1<Integer>() {
@Override
public void call(Integer integer) {
System.out.println("Success");
}
},
new Action1<Throwable>() {
@Override
public void call(Throwable throwable) {
System.out.println("I don't know if it was due to unsubscribing or the source is misbehaving");
}
});
until.onNext(1);vs. with PublishSubject<Integer> source = PublishSubject.create();
PublishSubject<Integer> until = PublishSubject.create();
source.take(1).toSingle()
.takeUntil(until.take(1).toSingle())
.subscribe(
new Action1<Integer>() {
@Override
public void call(Integer integer) {
System.out.println("Success");
}
},
new Action1<Throwable>() {
@Override
public void call(Throwable throwable) {
if (throwable instanceof CancellationException) {
System.out.println("It was canceled.");
} else {
System.out.println("Source didn't emit.");
}
}
});
until.onNext(1);The example I gave in the issue is probably the best example for me. We use something similar to this for lifecycle binding in android. When the lifecycle ends, it might unsubscribe this in the middle. For a normal error, we might show a generic "an error occurred" message. In the event that it's just the lifecycle ending, we don't want to react that way, and rather likely just want to do nothing at all or clean up resources. I'm fine with not using I thought about just specifying a message, but felt that @akarnokd's |
|
I'll add an overload for |
|
@hzsweers I actually typed "it's a big deal" but I was willing to say "it's not a big deal" (I corrected my previous comment). I prefer the Subject based approach, but I am not strongly opinionated about that. |
My point here is if Single.takeUntil(...) returns a Single that emits nothing, it should be |
|
I think "CancellationException" is the clearer reaction here. Remember the problems around the Observable.single() and how it is a source of problem to find out exactly who didn't signal? Here, you know that if takeUntil is tripped and not some upstream machinery ends up being empty. |
What if we want to add other operator that may return something doesn't signal? If it also emits |
|
What about a subclass of |
Maybe a more general question, should we add special exceptions for different operators, or we just use a general exception to indicate the same error? |
|
I think people should be aware of the implications of operators they use. Would |
CompositeException is fine and clear. It indicates there are multiple errors thrown. All classes in rx.exceptions are well defined, and it's very easy to connect them with the bad cases. I can just read the exception name and tell what my codes violate. But for If we can define clearly that when should throw CancellationException/CanceledNoSuchElementException(or whatever you propose), when should throw NoSuchElementException, I won't be against that. For now, the confusing thing for me is that if a Single doesn't signal, I may receive CancellationException or NoSuchElementException. |
|
I'm fine with either. I think we could be clear in the documentation, and subclassing would still allow downstream subscribers to treat it as a |
|
Added ping @akarnokd @stevegury |
|
👍 |
1 similar comment
|
👍 |
Add takeUntil support in Single
As discussed in #3708
This adds
takeUntil(Observable)andtakeUntil(Single)support inSingle. It was mostly just adapting the logic from the existingOperatorTakeUntiland adjusting it for accepting aSingleand sending aCancelattionExceptionin the event of a submission fromotherprior to a terminal event in the sourceSingle.Any feedback is appreciated it, this is my first time contributing an implementation to this project. Particularly wondering if it's worth keeping both overloads or if the user should just coerce their
otherto one type or ther other. Also particularly looking for feedback on what information to include in theCancellationException.