Skip to content
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

1.x: Add Single.onErrorResumeNext(Func) #3766

Conversation

artem-zinnatullin
Copy link
Contributor

Closes #3440, closes #3731, closes #3472 (whoa, 3 issues at a time!)

import rx.functions.Func1;
import rx.plugins.RxJavaPlugins;

public class SingleOperatorOnErrorResumeNext<T> implements Single.OnSubscribe<T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just renamed SingleOperatorOnErrorResumeNextViaSingle to something more general and it's now works with function instead of Single directly.

@artem-zinnatullin artem-zinnatullin force-pushed the single-on-error-resume-next-with-function branch from a2f5e89 to afd6649 Compare March 14, 2016 23:45

try {
unsubscribe();
resumeFunctionInCaseOfError.call(error).subscribe(child);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd separate catching the error and subscribing to the Single outside the try-catch.

@artem-zinnatullin
Copy link
Contributor Author

@akarnokd fixed your comments, btw, should I add @Experimental to these operators?

}

@Test
public void onErrorResumeNextViaFunctionShouldPreventNullFunction() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check/handle if the function returns a null Single.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test, but didn't handle this specifically in the operator, test will make sure that it won't be swallowed

@akarnokd
Copy link
Member

Yes, and copy over the experimental and since tags into the javadoc.

@artem-zinnatullin artem-zinnatullin force-pushed the single-on-error-resume-next-with-function branch from afd6649 to 0495044 Compare March 14, 2016 23:55
@akarnokd
Copy link
Member

👍 There is this new like option but do you get a notification for them?

@JakeWharton
Copy link
Contributor

No

On Mon, Mar 14, 2016, 8:10 PM David Karnok [email protected] wrote:

[image: 👍] There is this new like option but do you get a notification
for them?


You are receiving this because you are subscribed to this thread.

Reply to this email directly or view it on GitHub:
#3766 (comment)

@Override
public void onError(Throwable error) {
try {
resumeFunctionInCaseOfError.call(error).subscribe(child);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove unsubscribe()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's terminal state, we don't do unsubscribe() in onSuccess() too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If resumeFunctionInCaseOfError returns something like Observable.never.toSingle, we should still unsubscribe the original one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. I just found SafeSubscriber will do it.

@zsxwing
Copy link
Member

zsxwing commented Mar 15, 2016

👍

@stevegury
Copy link
Member

👍

stevegury added a commit that referenced this pull request Mar 15, 2016
…e-next-with-function

1.x: Add Single.onErrorResumeNext(Func)
@stevegury stevegury merged commit d36b626 into ReactiveX:1.x Mar 15, 2016
@artem-zinnatullin artem-zinnatullin deleted the single-on-error-resume-next-with-function branch March 15, 2016 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants