Skip to content

Fix get_macarthur example failure on R-universe - #48

Closed
nniiicc wants to merge 10 commits into
ropensci:masterfrom
nniiicc:master
Closed

nniiicc wants to merge 10 commits into
ropensci:masterfrom
nniiicc:master

Conversation

@nniiicc

@nniiicc nniiicc commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Wraps the get_macarthur() example in \dontrun{} and adds error handling for non-JSON HTTP responses, fixing the R CMD check failure on R-universe builds.

isaacOnline and others added 10 commits March 25, 2024 18:33
Three changes made: 1) R/macarthur.R:7-10 — Example wrapped in \dontrun{}, so it won't execute during R CMD check; 2)
R/macarthur.R:25-27 — Added inherits(response, "xml_document") check so a 404 returns NULL gracefully instead of crashing in fromJSON(); 3) man/get_macarthur.Rd — Regenerated with \dontrun{} wrapper
man/get_macarthur.Rd — Regenerated with \dontrun{} wrappe
Copilot AI review requested due to automatic review settings April 3, 2026 00:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR targets an R CMD check failure on R-universe by preventing the get_macarthur() example from running during checks and attempting to make MacArthur fetching more robust to non-JSON responses. It also includes broader scraper/pagination changes across several other sources and updates CI workflow action versions.

Changes:

  • Wrap get_macarthur() examples in \dontrun{} (roxygen + Rd) and modify MacArthur request/response handling.
  • Add/adjust pagination or multi-page fetching behavior for Open Society and Gates; adjust RSF scraping/parsing; add a warning for truncated Arnold results.
  • Update vcr cassette behavior in a couple tests and bump GitHub Actions versions in workflows.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
vignettes/awardFindR.Rmd Adjusts an example date range for multi-source search.
tests/testthat/test-osociety.R Changes vcr cassette recording mode.
tests/testthat/test-macarthur.R Changes vcr cassette recording mode.
R/rsf.R Reworks RSF award-page scraping to extract multiple scholars/institutions.
R/osociety.R Adds page iteration to collect multiple pages of Open Society results.
R/macarthur.R Wraps examples in \dontrun{} and changes MacArthur query + response parsing.
R/gates.R Switches to manual paging to fetch all Gates results.
R/arnold.R Fixes a comment typo and adds a message when results appear truncated.
man/get_macarthur.Rd Wraps the example in \dontrun{}.
DESCRIPTION Updates RoxygenNote.
.github/workflows/test-coverage.yaml Bumps GitHub Actions versions (checkout/cache/pandoc).
.github/workflows/check-full.yaml Bumps checkout action version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread R/macarthur.R
Comment on lines +28 to +34
response <- jsonlite::fromJSON(
rawToChar(
as.raw(
strtoi(response, 16L)
)
)
)

Copilot AI Apr 3, 2026

Copy link

Choose a reason for hiding this comment

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

request() returns parsed content via httr::content(): for JSON responses this is typically already a list. The new hex->raw decoding (strtoi(..., 16L)) will error for list or plain JSON text, and it will also corrupt valid JSON. Consider either (a) having request() return httr::content(..., as = "text") for this endpoint and pass that string to jsonlite::fromJSON(), or (b) if request() returns a list, skip fromJSON() entirely and use it directly. Also handle non-JSON responses by checking httr::headers() / content-type, not by attempting hex decoding.

Suggested change
response <- jsonlite::fromJSON(
rawToChar(
as.raw(
strtoi(response, 16L)
)
)
)
if (is.list(response)) {
response <- response
} else if (is.character(response)) {
response <- jsonlite::fromJSON(response)
} else {
return(NULL) # Unsupported/non-JSON response
}

Copilot uses AI. Check for mistakes.
Comment thread R/rsf.R
Comment on lines 32 to +38
# Getting details require seperate HTTP reqs for each award, ugh
df <- lapply(links, function(x) {
award <- request(x, "get", verbose)
award <- rvest::html_node(award, "div.content > header > div.u-nubbed")
program <- rvest::html_children(award)[1] %>% rvest::html_text(trim=TRUE)
program <- gsub("\t\t\t\t", "; ", program) # Separate with ;
title <- rvest::html_children(award)[2] %>% rvest::html_text(trim=TRUE)
title <- rvest::html_nodes(award, ".Post__titleGroup.u-margin__10") %>% rvest::html_text(trim = TRUE)

Copilot AI Apr 3, 2026

Copy link

Choose a reason for hiding this comment

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

The PR description says this change is just about wrapping get_macarthur() examples and adding error handling for non-JSON responses, but this PR also changes multiple other data sources (RSF, Open Society, Gates, Arnold) plus VCR test recording behavior and workflows. Please either update the PR description to reflect the broader scope or split these unrelated changes into separate PR(s) to reduce review/merge risk.

Copilot uses AI. Check for mistakes.
Comment thread R/osociety.R
Comment on lines +21 to +31
# Create object to iteratively add pages of results to
all_results <- list()

# Iterate through pages
page <- 1
while (length(results) > 0) {
page_url <- paste0(url, '&page=', page)

response <- request(page_url, "get", verbose)

results <- xml2::xml_find_all(response, "//div[@data-grants-database-single]")

Copilot AI Apr 3, 2026

Copy link

Choose a reason for hiding this comment

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

get_osociety() makes an initial request and parses results, but then the pagination loop starts with an empty all_results and re-requests page=1 without ever adding the initial page’s parsed results. This both duplicates the first request and risks dropping the first page (depending on whether page is 0/1-indexed). Consider seeding all_results from the initial results and then continuing from the next page index, rather than re-fetching page 1.

Suggested change
# Create object to iteratively add pages of results to
all_results <- list()
# Iterate through pages
page <- 1
while (length(results) > 0) {
page_url <- paste0(url, '&page=', page)
response <- request(page_url, "get", verbose)
results <- xml2::xml_find_all(response, "//div[@data-grants-database-single]")
results <- lapply(results, function(entry) {
institution <- xml2::xml_text(xml2::xml_find_first(entry, ".//h2"))
# Remove trailing and leading whitespace
institution <- gsub("^\\s+|\\s+$", "", institution)
id <- xml2::xml_text(xml2::xml_find_first(entry, ".//@id"))
year <- xml2::xml_integer(
xml2::xml_find_first(entry, ".//span[@class='a-grantsDatabase__value']"))
amount_xpath <- paste0(".//span[@class='a-grantsDatabase__value ",
"a-grantsDatabase__value--amount']")
amount <- xml2::xml_text(
xml2::xml_find_first(entry, amount_xpath))
# Remove $ and , in amounts (i.e. $1,000,000)
amount <- gsub("^\\$|,", "", amount)
# Make a data.frame of the labels and values that we can query later
info <- data.frame(
name = xml2::xml_text(
xml2::xml_find_all(entry, ".//span[@class='a-grantsDatabase__label']")),
value = xml2::xml_text(
xml2::xml_find_all(entry, ".//p[@class='a-grantsDatabase__text']")))
# Remove trailing and leading whitespace
info$value <- gsub("^\\s+|\\s+$", "", info$value)
# Now query it
program <- info$value[info$name == "Referring Program"][1]
description <- info$value[info$name == "Description"][1]
data.frame(institution, year, id, amount, program, description, keyword,
stringsAsFactors = FALSE)
})
# Seed with the initial page of results that was already fetched above
all_results <- results
# Iterate through remaining pages
page <- 2
while (TRUE) {
page_url <- paste0(url, '&page=', page)
response <- request(page_url, "get", verbose)
results <- xml2::xml_find_all(response, "//div[@data-grants-database-single]")
if (length(results)==0) {
break
}

Copilot uses AI. Check for mistakes.
Comment thread R/rsf.R
Comment on lines +50 to +54
# Function to get siblings until the next strong tag
get_siblings_until_next_strong <- function(node) {
siblings <- xml2::xml_siblings(node)
siblings <- siblings[which(rvest::html_name(siblings) != "strong")]
return(siblings)

Copilot AI Apr 3, 2026

Copy link

Choose a reason for hiding this comment

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

get_siblings_until_next_strong() doesn’t implement “until the next strong tag”: it calls xml_siblings() (which includes preceding and following siblings) and then simply filters out <strong> nodes, so it can pull in unrelated sections and reorder/mix content. Consider iterating through following siblings only (e.g., xml2::xml_next_sibling() / xml2::xml_find_all(node, "following-sibling::*")) and stop when the next <strong> is reached.

Suggested change
# Function to get siblings until the next strong tag
get_siblings_until_next_strong <- function(node) {
siblings <- xml2::xml_siblings(node)
siblings <- siblings[which(rvest::html_name(siblings) != "strong")]
return(siblings)
# Function to get following siblings until the next strong tag
get_siblings_until_next_strong <- function(node) {
if (length(node) == 0) {
return(node)
}
siblings <- xml2::xml_find_all(node[[1]], "following-sibling::*")
if (length(siblings) == 0) {
return(siblings)
}
next_strong_idx <- match("strong", rvest::html_name(siblings))
if (!is.na(next_strong_idx)) {
siblings <- siblings[seq_len(next_strong_idx - 1)]
}
siblings

Copilot uses AI. Check for mistakes.
Comment thread R/rsf.R
Comment on lines +46 to +60
# Get the nodes for the relevant sections
awarded_scholars_node <- strong_nodes[rvest::html_text(strong_nodes) == "Awarded Scholars: "]
other_external_scholars_node <- strong_nodes[rvest::html_text(strong_nodes) == "Other External Scholars: "]

# Function to get siblings until the next strong tag
get_siblings_until_next_strong <- function(node) {
siblings <- xml2::xml_siblings(node)
siblings <- siblings[which(rvest::html_name(siblings) != "strong")]
return(siblings)
}

# Extract the relevant text underneath the strong tags
awarded_scholars_siblings <- get_siblings_until_next_strong(awarded_scholars_node)
other_external_scholars_siblings <- get_siblings_until_next_strong(other_external_scholars_node)

Copilot AI Apr 3, 2026

Copy link

Choose a reason for hiding this comment

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

awarded_scholars_node / other_external_scholars_node can be an empty node set when an award page doesn’t contain that exact <strong> label. Passing an empty node set into xml2::xml_siblings() will error, which would make get_rsf() fragile. Add a guard (length check) and a fallback path (e.g., treat missing sections as zero entries) before attempting sibling extraction.

Copilot uses AI. Check for mistakes.
@nniiicc nniiicc closed this Apr 3, 2026
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.

3 participants