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 Retry plugin to http client #49

Merged
merged 2 commits into from
Oct 6, 2022
Merged

Add Retry plugin to http client #49

merged 2 commits into from
Oct 6, 2022

Conversation

dominikb
Copy link
Contributor

@dominikb dominikb commented Oct 5, 2022

Hi 👋

This addresses Issue #5 but two things are left to do:

  • Test that the Retry plugin can be disabled (Can you point me in the right direction on how I would go about this?)
  • Allow configuration of the plugin by passing an options array to it. Should this also be configurable in FreshBooksClientConfig? Maybe via a simple array?

Also, I'm trying to contribute to Hacktoberfest. Could you label the issue with hacktoberfest?

@amcintosh
Copy link
Owner

Hi!
Thanks for the PR! I have added the label.

Thinking about it again:

  • Testing would be difficult as you would need to somehow get a 5xx error out of the API. Also, this is basically testing if the retry plugin works, which really is something the plugin maintainers handle, so I think we can skip that.
  • Looking at the plugin, I think it's fine to have a simple enable/disable right now with the autoRetry config option you linked it to. The only thing I'm thinking to change is the number of retries. I like a default of 3, but I'd be open to that being something that can be changed.

@dominikb
Copy link
Contributor Author

dominikb commented Oct 6, 2022

Thanks for the response.

I've added an additional paramter to customize the number of retries. I've decided against validating the range of $retries because the retry plugin can handle 0 and negative values.

@amcintosh
Copy link
Owner

Thank you for this!

@amcintosh amcintosh merged commit e34bae5 into amcintosh:main Oct 6, 2022
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.

2 participants