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

Add a retries option as an alternative to the tries option #33

Closed

Conversation

jeromedalbert
Copy link

@jeromedalbert jeromedalbert commented Jun 9, 2021

Thanks for maintaining this cool gem!

Sometimes I prefer to think in terms of retries instead of tries. As in, "retry this code 3 times". This PR adds an optional retries option that does just that:

Retryable.retryable(retries: 3) { ... } # Retry 3 times, a.k.a try 4 times

This is just "syntactic sugar" as under the hood it just sets tries to retries + 1. But if you want to think in terms of retries, then I think this is a pretty useful option.

@nfedyashev
Copy link
Owner

@jeromedalbert Hi Jerome

Sorry for the delay. Thanks for your PR.

After thinking about this change for a while although there is nothing wrong with the code that you submitted I decided not to merge it. The motivation is:

  • this would make README look a bit weird with this mix of tries, retries options listed next to each other.
  • accepting it and not documenting in README - is not desirable because I don't want to have any undocumented features hidden in source code
  • perhaps the biggest reason is I'm trying to avoid avoid a new release if it adds little value for an average user. IMO, old release with lots of downloads is better because (1) existing users don't need to spend their time upgrading/checking CHANGELOG (2) new users have more confidence in old release with lots of downloads rather than a new release with just a few.

@nfedyashev nfedyashev closed this Jun 17, 2021
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

2 participants