search

civisanalytics / civis-r

Civis API Client for R: https://civisanalytics.com/products/civis-platform/
Other
16 stars 12 forks source link

replaced single-shot HTTP requests with RETRY logic #235

Closed jameslamb closed 4 years ago

jameslamb commented 4 years ago

In this PR, I'd like to propose swapping out calls to httr::POST() etc. with httr::RETRY(). It looks like you're already using RETRY() in a lot of places, which is awesome! This PR just changes the few remaining places where you weren't.

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 also noticed that you are mocking httr::VERB() but that it's never called in the code, so I removed those mocks.

I'm working on https://github.com/chircollab/chircollab20/issues/1 as part of Chicago R Collab, an R 'unconference' in Chicago.

Thanks for your time and consideration!

jameslamb commented 4 years ago

I pushed https://github.com/civisanalytics/civis-r/pull/235 to address the most recent review comments

mheilman commented 4 years ago

@jameslamb, would you care to merge this? (no rush, I just noticed this was still open)

jameslamb commented 4 years ago

@jameslamb, would you care to merge this? (no rush, I just noticed this was still open)

@mheilman I don't have permission to merge pull requests in this repository. That has to be done by a maintainer.

image

mheilman commented 4 years ago

Oh, my bad, sorry. I forgot about that! I thought you could merge if it was approved by a maintainer or something.