-
Notifications
You must be signed in to change notification settings - Fork 80
feat: add httr2_translate #797
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
base: main
Are you sure you want to change the base?
Conversation
|
CI has been tamed. Please let me know if there is anything I can do to move this along! I'd quite like to be able to use it as a debug option in if (isTRUE(getOption("arcgis.debug_curl")) {
cat(httr2::httr2_translate(req))
}This will help tremendously in getting reproducible examples from users as well as providing them to our distributed team of software engineers. Thanks! |
R/httr2-translate.R
Outdated
| if (length(cmd_parts) <= 2) { | ||
| paste(cmd_parts, collapse = " ") | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this branch actually necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is used so that the output is compatible with curl_translate() if curl is on its own line curl_translate() fails to read the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that but I think if remove the first branch then the results will still be the same.
I'd suggest renaming and refocussing cmd_parts to curl_args (i.e. adding curl and the url at the very end) to make this more clear.
|
Thank you so much for the feedback! I'll review this and make the requested changes. |
| headers <- req_get_headers(req, redacted = "reveal") | ||
|
|
||
| # if headers are present, add them using -H flag | ||
| if (!rlang::is_empty(headers)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, you don't need to first check if the vector is empty, if it is empty the for loop already doesn't do anything.
| } | ||
| } | ||
|
|
||
| known_curl_opts <- c( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest making this a separate function
| # if the headers aren't empty AND the content-type header is set | ||
| # we use that instead of what is inferred from the request object | ||
| if ( | ||
| !rlang::is_empty(headers) && ("content-type" %in% tolower(names(headers))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If x is empty, then "foo" %in% x will always return FALSE.
This PR closes #795 by adding an
httr2_translate()function.This function takes an
httr2_requestobject and does its best to faithfully create the equivalent curl request while respecting headers, cookies, options, request type etc.Created on 2025-08-20 with reprex v2.1.1