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

added retry logic to HTTP requests #1656

Merged
merged 4 commits into from
May 19, 2020
Merged

added retry logic to HTTP requests #1656

merged 4 commits into from
May 19, 2020

Conversation

jameslamb
Copy link
Contributor

Thanks for this awesome project!

In this PR, I'd like to propose swapping out calls to httr::GET(), httr::PATCH(), etc. with httr::RETRY(). This will make the package more resilient to transient problems like brief network outages or periods where the service(s) it hits are overwhelmed. In my experience, using retry logic almost always improves the user experience with HTTP clients.

I apologize for the whitespace changes. I have my editor set to trim excess horizontal whitespace when I save files. If you'd like me to revert those please let me know.

@cpsievert
Copy link
Collaborator

Thank you for the PR, and sorry for delayed response!

That'd be great if you could revert the whitespace changes. Also, I think you missed this call to httr::VERB()

@jameslamb
Copy link
Contributor Author

Thanks. I've reverted all of the whitespace changes and replaced the httr::VERB()

R/api_exports.R Outdated Show resolved Hide resolved
R/api_exports.R Outdated Show resolved Hide resolved
R/plotly_IMAGE.R Outdated Show resolved Hide resolved
@cpsievert
Copy link
Collaborator

Sorry for the delay in getting this merged @jameslamb, this is looking good, and I'd be happy to take it on. Just a couple minor nitpicks:

  • terminate_on was added in httr 1.3.0, so lets do httr (>= 1.3.0) in the DESCRIPTION file.
  • Please use trailing commas (instead of starting a new line with a comma).

@jameslamb
Copy link
Contributor Author

Sorry for the delay in getting this merged @jameslamb, this is looking good, and I'd be happy to take it on. Just a couple minor nitpicks:

  • terminate_on was added in httr 1.3.0, so lets do httr (>= 1.3.0) in the DESCRIPTION file.
  • Please use trailing commas (instead of starting a new line with a comma).

will do, thanks for the review!

@jameslamb
Copy link
Contributor Author

Ok I just made the requested changes in 1bf6a4a. I've also rebased to master to catch changes that have happened since this was opened.

@cpsievert
Copy link
Collaborator

Awesome, thank you! One last request: could you add a NEWS.md item (under improvements)?

@jameslamb
Copy link
Contributor Author

Sure! Just added in 0f1f181

I also fixed a missing comma from my last commit, which I think was the reason Travis was ❌

@cpsievert cpsievert merged commit c19594b into plotly:master May 19, 2020
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.

2 participants