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

replace GET() with RETRY() #68

Merged
merged 1 commit into from
Apr 21, 2020
Merged

replace GET() with RETRY() #68

merged 1 commit into from
Apr 21, 2020

Conversation

jameslamb
Copy link
Contributor

@jameslamb jameslamb commented Apr 18, 2020

Thanks for for this awesome project!

In this PR, I'd like to propose swapping out calls to httr::GET() 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'm working on chircollab/chircollab20#1 as part of Chicago R Collab, an R 'unconference' in Chicago.

@jtrecenti
Copy link
Member

Thanks! Just one question: Is that a way that httr::RETRY causes other unexpected error, or should it be exactly like httr::GET used many times? I'm not used to httr::RETRY, so I would like to be sure it cannot cause any unexpected problems before merging.

@jameslamb
Copy link
Contributor Author

Thanks! Just one question: Is that a way that httr::RETRY causes other unexpected error, or should it be exactly like httr::GET used many times? I'm not used to httr::RETRY, so I would like to be sure it cannot cause any unexpected problems before merging.

In my experience, httr::RETRY() behaves the same as httr::GET() for all practical purposes...it takes the same arguments (plus additional ones that are specific to retry like pause_cap) and always returns an httr response object. I've used RETRY() in my own packages for maybe 2 years and never saw a situation where it did something that GET(), POST(), etc. wouldn't do.

@jtrecenti jtrecenti merged commit a3f3b17 into curso-r:master Apr 21, 2020
@jtrecenti
Copy link
Member

thanks! 😄

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