replaced single-shot HTTP requests with RETRY()

[jameslamb]

Thanks for for this awesome project!

In this PR, I'd like to propose swapping out calls to httr::POST() 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. httr::RETRY() is awesome...it already implements exponential backoff and if you want you can customize things like how many retries it attempts and how long it's willing to wait between retries.

I'm working on chircollab/chircollab20#1 as part of Chicago R Collab, an R 'unconference' in Chicago.

Thanks for your time and consideration.

[codecov-commenter]

Codecov Report

Merging #113 into master will decrease coverage by 6.16%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
- Coverage   51.54%   45.37%   -6.17%     
==========================================
  Files          12       12              
  Lines         454      454              
==========================================
- Hits          234      206      -28     
- Misses        220      248      +28     

Impacted Files	Coverage Δ
R/package.R	58.82% <83.33%> (-8.83%)	⬇️
R/gh_whoami.R	41.17% <0.00%> (-58.83%)	⬇️
R/gh_response.R	42.37% <0.00%> (-25.43%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faf4206...547cf07. Read the comment docs.

[jameslamb]

I've rebased this to pick up the changes merged into this project in the time since this pull request was opened.

[jameslamb]

I just merged in the latest changes from master and fixed merge conflicts.

Are you open to this pull request? If not, I'd be happy to close it.

[krlmlr]

The tests are failing, is this a false positive or a limitation of RETRY()?

[gaborcsardi]

@krlmlr Seems like because there is no GH token in the PR.

[krlmlr]

Good catch, will try locally.

[krlmlr]

Can't rerun, this seems to be a special token. Do the tests work for you locally?

[gaborcsardi]

Can't rerun, this seems to be a special token. Do the tests work for you locally?

Yes.

[krlmlr]

I like the approach. Of course we could do better by reviewing the headers. Merging this will ease the pain of failing GHA tests due to rate limits.

[krlmlr]

Maybe we should use RETRY(quiet = FALSE) or RETRY(quiet = .progress) ?

[gaborcsardi]

This feels like a big change for me, and I think it would be better to make it optional and opt-in.

[krlmlr]

I'll test-drive it so that we can gain more experience here.

Opt-in won't fix the problems right away, we'd need to change all packages that use it to opt in.

[gaborcsardi]

What are the problems that this would fix right away?

[krlmlr]

I'm hoping that the functions in remotes that access the GitHub API will wait when the rate limit is exceeded, when they would otherwise fail.

[gaborcsardi]

I'm hoping that the functions in remotes that access the GitHub API will wait when the rate limit is exceeded, when they would otherwise fail.

remotes does not use gh for anything.

[jameslamb]

This PR has been open for 20+ months, and hasn't received a new comment in over a year. I'm going to close it and leave it to maintainers to make this change separately if you want.