Conversation
|
@jwokaty https://handle.test.datacite.org/10.82962/b9.bioc.multiassayexperiment |
0469acd to
e7bdd2d
Compare
|
Any updates on this @jwokaty ? |
There was a problem hiding this comment.
Pull request overview
This PR migrates the internal generateBiocPkgDOI() helper from httr to httr2 for creating DOIs via the DataCite REST API.
Changes:
- Replaced the POST implementation with an httr2 request pipeline (
request()+req_auth_basic()+ JSON body +req_perform()). - Simplified response handling to extract
data$iddirectly fromresp_body_json(). - Updated
NAMESPACEimports to drop unused httr/stringr imports and add required httr2 imports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
R/newBiocPkgDOI.R |
Rewrites DOI creation request/response code to use httr2 instead of httr. |
NAMESPACE |
Adjusts imports to align with the httr2-based implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| response <- request(base_url) |> | ||
| req_auth_basic(username, password) |> | ||
| req_headers("Content-Type" = "application/vnd.api+json") |> | ||
| req_body_json(payload) |> | ||
| req_perform() |
There was a problem hiding this comment.
Applied in c4041e7. Changed to req_body_json(payload, type = "application/vnd.api+json") so the vendor media type is set directly and not overwritten. Also removed the now-unused req_headers() call, dropped the stale jsonlite imports, and added resp_check_status() after req_perform() to surface HTTP errors clearly before parsing the response body.
There was a problem hiding this comment.
Thanks for taking a look. The change removed the Content-Type header from the request by only using req_body_json. I have added it back in.
|
|
||
| if (response$status_code >= 200 && response$status_code < 300) { | ||
| response_text <- httr::content(response, "text") | ||
| response_json <- jsonlite::fromJSON(response_text) | ||
| return(response_json$data$id) | ||
| } else { | ||
| httr::message_for_status(response) | ||
| } | ||
| resp_body_json(response)$data$id |
There was a problem hiding this comment.
This was already applied in c4041e7 — resp_check_status(response) is called on line 82, before parsing $data$id, so any authentication or validation failures surface as informative HTTP errors rather than a confusing parse failure.
|
@LiNk-NY I'm sorry I missed this multiple times. I can review when I return next week. |
- Add resp_check_status() to surface HTTP errors before parsing body - Update NAMESPACE to add resp_check_status import Agent-Logs-Url: https://github.com/seandavi/BiocPkgTools/sessions/449565a2-c285-4206-8041-f299c3adb2c5 Co-authored-by: seandavi <92435+seandavi@users.noreply.github.com>
Hi Andres, @jwokaty
I don't have a Datacite account for testing the re-written
generateBiocPkgDOIfunction. Could you review and make sure that the changes work?
You can also share the details via bitwarden.
Thank you!