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

Switch from httr to httr2 #174

Merged
merged 25 commits into from
Feb 14, 2023
Merged

Switch from httr to httr2 #174

merged 25 commits into from
Feb 14, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Feb 6, 2023

And add rate limiting. Fixes #67.

R/gh_request.R Show resolved Hide resolved
R/gh_response.R Show resolved Hide resolved
R/gh_response.R Show resolved Hide resolved
R/gh_response.R Outdated Show resolved Hide resolved
@hadley hadley marked this pull request as ready for review February 7, 2023 14:20
@hadley
Copy link
Member Author

hadley commented Feb 7, 2023

@gaborcsardi if we decided to go down this path, I'll consider testing more broadly, doing some more refactoring and adding tests to get better coverage. But I have invested relatively little time at this point, so it would be easier enough to give it up.

A few reasons why it'd be worthwhile to switch to httr2:

  • Adding retry with delay is a single line
  • last_response() and last_request() make debugging in downstream packages much easier
  • Could likely do further refactoring to use req_template() (but I think that'll require some httr2 changes because of the way that gh puts unmatched args in the body)

Copy link
Member

@gaborcsardi gaborcsardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with switching to httr2, it also helps httr2 to get more usage.
Looks good, I left some comments. In particular, we could make sure that returning list() for an empty response does not break anything in usethis.

Some tests cases are failing for me, I think these might be fragile.

Failure (test-gh_whoami.R:10): whoami works in presence of PAT
res\[\["scopes"\]\] does not match "\\buser\\b".
Actual value: "delete:packages, delete_repo, read:org, repo, workflow, write:packages"
Backtrace:
 1. testthat::expect_match(res[["scopes"]], "\\buser\\b")
      at test-gh_whoami.R:10:2
 2. testthat:::expect_match_(...)
Error (test-mock-repos.R:33): can POST, PATCH, and DELETE
<github_error/http_error_404/rlang_error/error/condition>
Error in `gh_process_response(raw)`: GitHub API error (404): Not Found
x URL not found: <https://api.github.com/gists>
i Read more at <https://docs.github.com/rest/reference/gists#create-a-gist>
Backtrace:
 1. gh::gh(...)
      at test-mock-repos.R:33:2
 2. gh:::gh_process_response(raw)
      at gh/R/gh.R:176:2
Error (test-mock-repos.R:33): can POST, PATCH, and DELETE
<github_error/http_error_404/rlang_error/error/condition>
Error in `gh_process_response(raw)`: GitHub API error (404): Not Found
x URL not found: <https://api.github.com/gists>
i Read more at <https://docs.github.com/rest/reference/gists#create-a-gist>
Backtrace:
 1. gh::gh(...)
      at test-mock-repos.R:33:2
 2. gh:::gh_process_response(raw)
      at gh/R/gh.R:176:2

DESCRIPTION Show resolved Hide resolved
R/gh.R Outdated Show resolved Hide resolved
R/gh_response.R Show resolved Hide resolved
R/gh_response.R Outdated Show resolved Hide resolved
@hadley
Copy link
Member Author

hadley commented Feb 8, 2023

Hmmm, those tests assume that you have a PAT created with our default scopes. I can skip if the scopes aren't as expected, if you want.

@gaborcsardi
Copy link
Member

Yeah, I think we need to skip, or add back the GH_TESTING token on the CI and skip if it is not set, because GITHUB_TOKEN might not have the same scopes for all the developers and also for the CI jobs.

R/gh.R Outdated Show resolved Hide resolved
R/gh.R Outdated Show resolved Hide resolved
@hadley hadley changed the title First pass at using httr2 Switch from httr to httr2 Feb 13, 2023
R/gh_rate_limit.R Outdated Show resolved Hide resolved
@hadley hadley merged commit d892f5a into main Feb 14, 2023
@hadley hadley deleted the httr2 branch February 14, 2023 16:33
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.

Rate limit helpers
2 participants