Skip to content

Gather all failures in roxygen_test.R #3012

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.

Sign up for GitHub

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

Open
MichaelChirico wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Gather all failures in roxygen_test.R #3012

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09a20a7
Gather all failures in roxygen_test.R
MichaelChirico Jan 26, 2026
0cd1c0e
Gather all failures in roxygen_test.R
MichaelChirico Jan 26, 2026
76579cc
unnest
MichaelChirico Jan 26, 2026
0a0da18
more verbose for readability
MichaelChirico Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 46 additions & 7 deletions .dev/roxygen_test.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ stopifnot(file.copy("man", tempdir(), recursive = TRUE))
old_files <- list.files(old_dir, pattern = "\\.Rd$")
new_dir <- "man"

.Last <- function() unlink(old_dir, recursive = TRUE)
.Last <- function() {
if (!dir.exists(old_dir)) {
return(invisible())
}
unlink(new_dir, recursive = TRUE)
file.copy(old_dir, ".", recursive = TRUE)
unlink(old_dir, recursive = TRUE)
}

# Rd2txt() prints to its out= argument, so we'd have to compare file contents;
# plain parse_Rd() keeps srcref info that encodes the file path, which as.character() strips.
Expand All @@ -17,19 +24,41 @@ normalize_rd <- function(rd_file) as.character(parse_Rd(rd_file))
rd_equal <- function(f1, f2) isTRUE(all.equal(normalize_rd(f1), normalize_rd(f2)))

check_roxygenize_idempotent <- function(LOCALE) {
tryCatch(Sys.setlocale("LC_COLLATE", LOCALE), warning = stop)
set_locale_res <- tryCatch(Sys.setlocale("LC_COLLATE", LOCALE), warning = identity, error = identity)
if (inherits(set_locale_res, "condition")) {
message(sprintf(
"Skipping LOCALE=%s because it is not supported: %s",
LOCALE, conditionMessage(set_locale_res)
))
return(TRUE)
}

# Ensure man/ is in its original state before running roxygenize()
unlink(new_dir, recursive = TRUE)
stopifnot(file.copy(old_dir, ".", recursive = TRUE))

suppressMessages(roxygenize()) # 'loading lintr'

new_files <- list.files(new_dir, pattern = "\\.Rd$")

any_failed <- FALSE

old_not_new <- setdiff(old_files, new_files)
if (length(old_not_new) > 0L) {
stop("Found saved .Rd files gone from a fresh run of roxygenize(): ", toString(old_not_new))
cat(sprintf(
"Found saved .Rd files gone from a fresh run of roxygenize() in LOCALE=%s: %s\n",
LOCALE, toString(old_not_new)
))
any_failed <- TRUE
}

new_not_old <- setdiff(new_files, old_files)
if (length(new_not_old) > 0L) {
stop("Found new .Rd files from a fresh run of roxygenize(): ", toString(new_not_old))
cat(sprintf(
"Found new .Rd files from a fresh run of roxygenize() in LOCALE=%s: %s\n",
LOCALE, toString(new_not_old)
))
any_failed <- TRUE
}

for (file in new_files) {
Expand All @@ -38,16 +67,26 @@ check_roxygenize_idempotent <- function(LOCALE) {
if (rd_equal(old_file, new_file)) {
next
}
cat(sprintf("roxygenize() output differs from saved output for %s.\n", file))
cat(sprintf("roxygenize() output differs from saved output for %s in LOCALE=%s.\n", file, LOCALE))
cat("Here's the 'diff' comparison of the two files:\n")
cat(" [---]: saved output in man/ directory\n")
cat(" [+++]: roxygenize() output of R/ sources\n")
system2("diff", c("--unified", old_file, new_file))
stop("Failed in LOCALE=", LOCALE, ".", call. = FALSE)
any_failed <- TRUE
}

!any_failed
}

# Run the check in a few locales to ensure there's no idempotency issues w.r.t. sorting, too
all_locales_passed <- TRUE
for (LOCALE in c("C", "en_US.utf8", "hu_HU.utf8", "ja_JP.utf8")) {
check_roxygenize_idempotent(LOCALE)
if (!check_roxygenize_idempotent(LOCALE)) {
all_locales_passed <- FALSE
}
}
Comment on lines 82 to 87
Copy link
Collaborator

@Bisaloo Bisaloo Jan 30, 2026

Choose a reason for hiding this comment

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

The switch between looking at "what failed" in check_roxygenize_idempotent() vs "what passed" here is not the most straightforward to resonate about IMO.

Copy link
Collaborator Author

@MichaelChirico MichaelChirico Jan 30, 2026

Choose a reason for hiding this comment

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

Yea I think that's me being too cute to save 2 lines.

Copy link
Collaborator

@Bisaloo Bisaloo Jan 31, 2026

Choose a reason for hiding this comment

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

No, sorry, I wasn't clear. I think what you changed was perfectly reasonable.

I highlighted the fact we use any_failed inside check_roxygenize_idempotent() to all_locales_passed outside. It requires some mental gymnastics when thinking about the code, to make the boolean negation.

I would use all_passed (rather than any_failed) inside check_roxygenize_idempotent() or any_locale_failed in the for loop here.

Does this make sense?

(More generally, and slightly off-topic, I would likely generally consider that using ! in an return() is likely a code smell for readability. Maybe a linter could make sense.)


if (!all_locales_passed) {
message("roxygenize() check failed.")
q(status = 1)
}
Loading