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 httr with httr2 #2600

Merged
merged 5 commits into from
May 30, 2024
Merged

Replace httr with httr2 #2600

merged 5 commits into from
May 30, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented May 29, 2024

Also allows us to eliminate memoise since we can rely on httr2's HTTP caching instead :)

Since I was in there, I also slightly tweaked the feedback from build_favicons(), since I was confused that it didn't print anything on completion.

Also allows us to eliminate memoise since we can rely on httr2's HTTP caching instead :)

Since I was in there, I also slightly tweaked the feedback from `build_favicons()`, since I was confused that it didn't print anything on completion.
@hadley hadley requested a review from maelle May 29, 2024 20:10
Copy link

github-actions bot commented May 29, 2024

DESCRIPTION Outdated Show resolved Hide resolved
withCallingHandlers(
resp <- httr2::req_perform(req),
error = function(e) {
cli::cli_abort("API request failed.", parent = e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

indicate which API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I contemplated that too, but it ends up just echoing the url in the previous message, and the error will have build_favicon() in it, so I think it's pretty clear. (Plus I've never actually seen this fail, which is pretty remarkable)


if (!httr::http_error(cran_url)) {
req <- httr2::request(cran_url)
req <- httr2::req_cache(req, path = http_cache_dir(), max_age = 86400)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is that age? maybe add a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just assume everyone should know how many seconds are in a day 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok ok I'll try to remember about this magic number 😅

req <- httr::RETRY("HEAD", bioc_url, quiet = TRUE)
if (!httr::http_error(req) && !grepl("removed-packages", req$url)) {
req <- httr2::request(bioc_url)
req <- httr2::req_cache(req, path = http_cache_dir(), max_age = 86400)
Copy link
Collaborator

Choose a reason for hiding this comment

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

could http_cache_dir() be called pkgdown_http_cache_dir()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've refactored this to avoid the problem (and add a comment about the number of seconds 😄)

R/build-favicons.R Outdated Show resolved Hide resolved
if (!httr::http_error(req) && !grepl("removed-packages", req$url)) {
req <- httr2::request(bioc_url)
req <- httr2::req_cache(req, path = http_cache_dir(), max_age = 86400)
req <- httr2::req_error(req, function(resp) FALSE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

pkgdown imports magrittr but I suppose not having req as a pipeline is a preference? I was a bit surprised by the repeated explicit assignments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(here and in the other calls to httr2 functions)

Copy link
Member Author

Choose a reason for hiding this comment

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

Using a pipe inside a package just feels weird to me. I might eliminate the relatively few remaining uses in pkgdown, since that would allow us to drop another dependency (which is useful since we're skating very close to the limit of 22)

@hadley hadley merged commit db802a4 into main May 30, 2024
14 checks passed
@hadley hadley deleted the httr2 branch May 30, 2024 13:13
SebKrantz pushed a commit to SebKrantz/pkgdown that referenced this pull request Jun 1, 2024
Also allows us to eliminate memoise since we can rely on httr2's HTTP caching instead :)

Since I was in there, I also slightly tweaked the feedback from `build_favicons()`, since I was confused that it didn't print anything on completion.
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.

2 participants