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

Operator Retry with predicate #1271

Merged
merged 2 commits into from
May 28, 2014

Conversation

akarnokd
Copy link
Member

Operator Retry with predicate + 1 overload for Issue #1214.

The Func1 version is simply named retry, the Func2 version is named retryIf due to lack of ideas and because to avoid overload resolution issues. I also had to open up the OperatorRetryTest classes a bit.

@cloudbees-pull-request-builder

RxJava-pull-requests #1169 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Member

Thanks for implementing this. Should we just keep one of those two instead of both so we don't have to deal with the naming/overload issue?

@akarnokd
Copy link
Member Author

I don't know, which one to keep?

@benjchristensen
Copy link
Member

Probably the one that includes attempts? This is the advanced use case for retry, so it should probably provide both attempts and error. It just feels awkward to need to come up with a new name just so we can have one with attempts and one without.

@headinthebox Do you have an opinion on this?

@headinthebox
Copy link
Contributor

Agree with @benjchristensen. If you are using this one, you (should) know what you are doing.

@cloudbees-pull-request-builder

RxJava-pull-requests #1176 SUCCESS
This pull request looks good

benjchristensen added a commit that referenced this pull request May 28, 2014
@benjchristensen benjchristensen merged commit 7e8fc57 into ReactiveX:master May 28, 2014
@benjchristensen
Copy link
Member

Thanks @akarnokd and @headinthebox for the review.

@akarnokd akarnokd deleted the OperatorRetryWithPredicate branch May 28, 2014 21:51
@benjchristensen benjchristensen mentioned this pull request Jun 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants