ggmap icon indicating copy to clipboard operation
ggmap copied to clipboard

replace httr::GET() with httr::RETRY()

Open ataustin opened this issue 4 years ago • 2 comments

Thanks for for this awesome package! I'm working on https://github.com/chircollab/chircollab20/issues/1 as part of Chicago R Collab.

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.

Thanks for your consideration!

Before you open your PR

  • [X] Did you run R CMD CHECK?
  • [X] Did you run roxygen2::roxygenise(".")?

Thanks for contributing!

ataustin avatar Apr 18 '20 19:04 ataustin

Cool. A few thoughts.

  1. Would you mind updating the NEWS.md to alert users?
  2. In your experience, how often is it retrying? Since Google follows a (small) fee for service mode, should we be concerned about retries?

dkahle avatar Apr 20 '20 04:04 dkahle

Interesting point about the fees! The default behavior of RETRY is to attempt only 3 times total before giving up. I'd expect package users to keep hitting the API if their first attempt fails regardless, so RETRY could be a courtesy to them. There is also a terminate_on argument which we could use to specify status codes for which the RETRY would automatically quit.

Let me know if this is acceptable, and if there are any status codes that you would like to kill the RETRY. I'll make any required changes, then update NEWS.md and push.

ataustin avatar Apr 22 '20 02:04 ataustin