From 589d21cbcd7a986d18e94f50ab29550fc384d207 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 4 Apr 2022 17:41:10 -0500 Subject: [PATCH 01/18] Implement warn_roxy_tag() and use in tag-parsers Ensure that markdown always checks for matched `{` --- NAMESPACE | 1 + R/markdown-state.R | 6 ++ R/rd-raw.R | 2 +- R/tag-parser.R | 104 ++++++++++++--------- R/tag.R | 11 +++ man/load_options.Rd | 2 - man/roxy_tag.Rd | 3 + tests/testthat/_snaps/block.md | 14 +++ tests/testthat/_snaps/namespace.md | 26 ++++++ tests/testthat/_snaps/rd-inherit.md | 4 + tests/testthat/_snaps/rd-raw.md | 14 +++ tests/testthat/_snaps/tag-parser.md | 138 ++++++++++++++++++++++++++++ tests/testthat/test-block.R | 16 ++-- tests/testthat/test-namespace.R | 25 +++-- tests/testthat/test-rd-inherit.R | 5 +- tests/testthat/test-rd-raw.R | 12 +-- tests/testthat/test-tag-parser.R | 54 ++++++++--- 17 files changed, 348 insertions(+), 89 deletions(-) create mode 100644 tests/testthat/_snaps/namespace.md create mode 100644 tests/testthat/_snaps/rd-raw.md create mode 100644 tests/testthat/_snaps/tag-parser.md diff --git a/NAMESPACE b/NAMESPACE index 2f9491d8f..ed0de854c 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -238,6 +238,7 @@ export(tag_words) export(tag_words_line) export(update_collate) export(vignette_roclet) +export(warn_roxy_tag) import(rlang) import(stringr) importFrom(R6,R6Class) diff --git a/R/markdown-state.R b/R/markdown-state.R index ca76dc740..975f4ecd5 100644 --- a/R/markdown-state.R +++ b/R/markdown-state.R @@ -16,6 +16,12 @@ markdown_on <- function(value = NULL) { return(isTRUE(markdown_env$`markdown-support`)) } +local_markdown <- function(env = parent.frame()) { + old <- markdown_env$`markdown-support` + markdown_on(TRUE) + withr::defer(markdown_on(FALSE), envir = env) +} + markdown_activate <- function(tags) { ## markdown on/off based on global flag and presence of @md & @nomd diff --git a/R/rd-raw.R b/R/rd-raw.R index ef82b2ed7..6dc49a45d 100644 --- a/R/rd-raw.R +++ b/R/rd-raw.R @@ -29,6 +29,6 @@ roxy_tag_eval <- function(tag, env) { out } }, error = function(e) { - roxy_tag_warning(tag, "failed with error:\n", e$message) + warn_roxy_tag(tag, "failed to execute", parent = e) }) } diff --git a/R/tag-parser.R b/R/tag-parser.R index b5bc5e00f..cfeba3493 100644 --- a/R/tag-parser.R +++ b/R/tag-parser.R @@ -18,9 +18,11 @@ NULL #' @rdname tag_parsers tag_value <- function(x) { if (str_trim(x$raw) == "") { - roxy_tag_warning(x, "requires a value") + warn_roxy_tag(x, "requires a value") + NULL } else if (!rdComplete(x$raw, is_code = FALSE)) { - roxy_tag_warning(x, "mismatched braces or quotes") + warn_roxy_tag(x, "has mismatched braces or quotes") + NULL } else { x$val <- str_trim(x$raw) x @@ -31,9 +33,11 @@ tag_value <- function(x) { #' @rdname tag_parsers tag_inherit <- function(x) { if (str_trim(x$raw) == "") { - roxy_tag_warning(x, "requires a value") + warn_roxy_tag(x, "requires a value") + NULL } else if (!rdComplete(x$raw, is_code = FALSE)) { - roxy_tag_warning(x, "mismatched braces or quotes") + warn_roxy_tag(x, "has mismatched braces or quotes") + NULL } else { pieces <- str_split(str_trim(x$raw), "\\s+")[[1]] fields <- pieces[-1] @@ -46,8 +50,7 @@ tag_inherit <- function(x) { } else { unknown <- setdiff(fields, all) if (length(unknown) > 0) { - types <- paste0(unknown, collapse = ", ") - roxy_tag_warning(x, "Unknown inherit type: ", types) + warn_roxy_tag(x, "inherits from unknown type {.str unknown}") fields <- intersect(fields, all) } } @@ -65,11 +68,14 @@ tag_inherit <- function(x) { #' @rdname tag_parsers tag_name <- function(x) { if (str_trim(x$raw) == "") { - roxy_tag_warning(x, "requires a name") + warn_roxy_tag(x, "requires a name") + NULL } else if (!rdComplete(x$raw, is_code = FALSE)) { - roxy_tag_warning(x, "mismatched braces or quotes") + warn_roxy_tag(x, "has mismatched braces or quotes") + NULL } else if (str_count(x$raw, "\\s+") > 1) { - roxy_tag_warning(x, "should have only a single argument") + warn_roxy_tag(x, "must have only one argument") + NULL } else { x$val <- str_trim(x$raw) x @@ -84,11 +90,14 @@ tag_name <- function(x) { #' @param markdown Should the second part be parsed as markdown? tag_two_part <- function(x, first, second, required = TRUE, markdown = TRUE) { if (str_trim(x$raw) == "") { - roxy_tag_warning(x, "requires a value") + warn_roxy_tag(x, "requires a value") + NULL } else if (required && !str_detect(x$raw, "[[:space:]]+")) { - roxy_tag_warning(x, "requires ", first, " and ", second) + warn_roxy_tag(x, "requires {first} and {second}") + NULL } else if (!rdComplete(x$raw, is_code = FALSE)) { - roxy_tag_warning(x, "mismatched braces or quotes") + warn_roxy_tag(x, "has mismatched braces or quotes") + NULL } else { pieces <- str_split_fixed(str_trim(x$raw), "[[:space:]]+", 2) pieces[is.na(pieces)] <- "" @@ -117,18 +126,21 @@ tag_name_description <- function(x) { #' @param min,max Minimum and maximum number of words tag_words <- function(x, min = 0, max = Inf) { if (!rdComplete(x$raw, is_code = FALSE)) { - return(roxy_tag_warning(x, "mismatched braces or quotes")) + warn_roxy_tag(x, "has mismatched braces or quotes") + return(NULL) } words <- str_split(str_trim(x$raw), "\\s+")[[1]] if (length(words) < min) { - roxy_tag_warning(x, " needs at least ", min, " words") + warn_roxy_tag(x, "must have at least {min} words") + NULL } else if (length(words) > max) { - roxy_tag_warning(x, " can have at most ", max, " words") + warn_roxy_tag(x, "must have at most {max} words") + NULL + } else { + x$val <- words + x } - - x$val <- words - x } #' @export @@ -137,9 +149,11 @@ tag_words_line <- function(x) { x$val <- str_trim(x$raw) if (str_detect(x$val, "\n")) { - roxy_tag_warning(x, "may only span a single line") + warn_roxy_tag(x, "may only span a single line") + NULL } else if (!rdComplete(x$val, is_code = FALSE)) { - roxy_tag_warning(x, "mismatched braces or quotes") + warn_roxy_tag(x, "has mismatched braces or quotes") + NULL } else { x$val <- str_split(x$val, "\\s+")[[1]] x @@ -152,7 +166,8 @@ tag_toggle <- function(x) { x$val <- str_trim(x$raw) if (x$val != "") { - roxy_tag_warning(x, "has no parameters") + warn_roxy_tag(x, "has no parameters") + NULL } else { x } @@ -162,14 +177,16 @@ tag_toggle <- function(x) { #' @rdname tag_parsers tag_code <- function(x) { if (str_trim(x$raw) == "") { - roxy_tag_warning(x, "requires a value") + warn_roxy_tag(x, "requires a value") + NULL } else { tryCatch({ parse(text = x$raw) x$val <- x$raw x }, error = function(e) { - roxy_tag_warning(x, "code failed to parse.\n", e$message) + warn_roxy_tag(x, "failed to parse", parent = e) + NULL }) } } @@ -179,12 +196,14 @@ tag_code <- function(x) { #' @rdname tag_parsers tag_examples <- function(x) { if (str_trim(x$raw) == "") { - return(roxy_tag_warning(x, "requires a value")) + warn_roxy_tag(x, "requires a value") + return(NULL) } x$val <- escape_examples(gsub("^\n", "", x$raw)) if (!rdComplete(x$val, is_code = TRUE)) { - roxy_tag_warning(x, "mismatched braces or quotes") + warn_roxy_tag(x, "has mismatched braces or quotes") + NULL } else { x } @@ -194,39 +213,42 @@ tag_examples <- function(x) { #' @rdname tag_parsers tag_markdown <- function(x) { if (str_trim(x$raw) == "") { - return(roxy_tag_warning(x, "requires a value")) + warn_roxy_tag(x, "requires a value") + NULL + } else { + x$val <- markdown_if_active(x$raw, x) + x } - - x$val <- markdown_if_active(x$raw, x) - x } #' @export #' @rdname tag_parsers tag_markdown_with_sections <- function(x) { if (str_trim(x$raw) == "") { - return(roxy_tag_warning(x, "requires a value")) + warn_roxy_tag(x, "requires a value") + return(NULL) } x$val <- markdown_if_active(x$raw, x, sections = TRUE) - for (i in seq_along(x$val)) { - if (!rdComplete(x$val[i], is_code = FALSE)) { - roxy_tag_warning(x, "mismatched braces or quotes") - x$val[i] <- "" - } else { - x$val[i] <- str_trim(x$val[i]) - } - } - x } markdown_if_active <- function(text, tag, sections = FALSE) { if (markdown_on()) { - markdown(text, tag, sections) + out <- markdown(text, tag, sections) + + for (i in seq_along(out)) { + if (!rdComplete(out[[i]], is_code = FALSE)) { + warn_roxy_tag(tag, "has mismatched braces or quotes") + out[[i]] <- "" + } else { + out[[i]] <- str_trim(out[[i]]) + } + } + out } else { if (!rdComplete(text, is_code = FALSE)) { - roxy_tag_warning(tag, "mismatched braces or quotes") + warn_roxy_tag(tag, "has mismatched braces or quotes") "" } else { str_trim(text) diff --git a/R/tag.R b/R/tag.R index d502152e2..f9ba5578a 100644 --- a/R/tag.R +++ b/R/tag.R @@ -28,6 +28,10 @@ roxy_tag <- function(tag, raw, val = NULL, file = NA_character_, line = NA_integ ) } +roxy_test_tag <- function(raw = "", val = NULL) { + roxy_tag("test", raw = raw, val = val, file = "test.R", line = 1) +} + #' @rdname roxy_tag #' @param x A tag #' @export @@ -91,3 +95,10 @@ print.roxy_tag <- function(x, ...) { roxy_tag_warning <- function(x, ...) { roxy_warning(file = x$file, line = x$line, "@", x$tag, " ", ...) } + +#' @export +#' @rdname roxy_tag +warn_roxy_tag <- function(tag, message, ...) { + message[[1]] <- paste0("[", tag$file, ":", tag$line, "] @", tag$tag, " ", message[[1]]) + cli::cli_warn(message, ..., .envir = parent.frame()) +} diff --git a/man/load_options.Rd b/man/load_options.Rd index 7e2005825..5aaa95ced 100644 --- a/man/load_options.Rd +++ b/man/load_options.Rd @@ -31,8 +31,6 @@ Options in \code{man/roxygen/meta.R} override those present in \code{DESCRIPTION \item \code{markdown} \verb{}: translate markdown syntax to Rd? \item \code{r6} \verb{}: document R6 classes? \item \code{current_package} \verb{} (read only): name of package being documented. -\item \code{current_path} \verb{} (read only): full path of file being documented. -\item \code{current_file} \verb{} (read only): name of file being documented. \item \code{rd_family_title} \verb{}: overrides for \verb{@family} titles. See the \emph{rd} vignette for details: \code{vignette("rd", package = "roxygen2")} } diff --git a/man/roxy_tag.Rd b/man/roxy_tag.Rd index 32ae1a8c1..79c8ff17e 100644 --- a/man/roxy_tag.Rd +++ b/man/roxy_tag.Rd @@ -4,6 +4,7 @@ \alias{roxy_tag} \alias{roxy_tag_parse} \alias{roxy_tag_warning} +\alias{warn_roxy_tag} \title{\code{roxy_tag} S3 constructor} \usage{ roxy_tag(tag, raw, val = NULL, file = NA_character_, line = NA_integer_) @@ -11,6 +12,8 @@ roxy_tag(tag, raw, val = NULL, file = NA_character_, line = NA_integer_) roxy_tag_parse(x) roxy_tag_warning(x, ...) + +warn_roxy_tag(tag, message, ...) } \arguments{ \item{tag}{Tag name. Arguments starting with \code{.} are reserved for internal diff --git a/tests/testthat/_snaps/block.md b/tests/testthat/_snaps/block.md index 02e64f73d..199946b87 100644 --- a/tests/testthat/_snaps/block.md +++ b/tests/testthat/_snaps/block.md @@ -13,3 +13,17 @@ $topic f $alias f +# errors are propagated + + [:4] @eval failed to execute + Caused by error in `foo()`: + ! Uhoh + +# must return non-NA string + + [:3] @eval did not evaluate to a string + +--- + + [:3] @eval result contained NA + diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md new file mode 100644 index 000000000..e033d2a5d --- /dev/null +++ b/tests/testthat/_snaps/namespace.md @@ -0,0 +1,26 @@ +# poorly formed importFrom throws error + + [:2] @importFrom must have at least 2 words + +# rawNamespace must be valid code + + [:2] @rawNamespace failed to parse + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: a + + ^ + +# evalNamespace generates warning when code is invalid + + [:2] @evalNamespace failed to parse + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: a + + ^ + +# evalNamespace generates warning when code raises error + + [:2] @evalNamespace failed to execute + Caused by error: + ! Uhoh + diff --git a/tests/testthat/_snaps/rd-inherit.md b/tests/testthat/_snaps/rd-inherit.md index 22dd35483..4f338a130 100644 --- a/tests/testthat/_snaps/rd-inherit.md +++ b/tests/testthat/_snaps/rd-inherit.md @@ -5,6 +5,10 @@ "sha1"} } +# warns on unknown inherit type + + [:2] @inherit inherits from unknown type "unknown" + # warns if can't find section @inheritSection failed in topic "b". diff --git a/tests/testthat/_snaps/rd-raw.md b/tests/testthat/_snaps/rd-raw.md new file mode 100644 index 000000000..f08515e82 --- /dev/null +++ b/tests/testthat/_snaps/rd-raw.md @@ -0,0 +1,14 @@ +# evalRd must be valid code + + [:2] @evalRd failed to parse + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: a + + ^ + +# error-ful evalRd generates warning + + [:2] @evalRd failed to execute + Caused by error: + ! Uhoh + diff --git a/tests/testthat/_snaps/tag-parser.md b/tests/testthat/_snaps/tag-parser.md new file mode 100644 index 000000000..4de62d2c6 --- /dev/null +++ b/tests/testthat/_snaps/tag-parser.md @@ -0,0 +1,138 @@ +# tags containing only whitespace generate warning + + Code + tag <- roxy_test_tag(" ") + expect_parse_failure(tag_value(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_inherit(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_name(tag)) + Output + + Warning: + [test.R:1] @test requires a name + Code + expect_parse_failure(tag_two_part(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_name_description(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_code(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_examples(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_markdown(tag)) + Output + + Warning: + [test.R:1] @test requires a value + Code + expect_parse_failure(tag_markdown_with_sections(tag)) + Output + + Warning: + [test.R:1] @test requires a value + +# tags check for mismatched parents gives useful warnings + + Code + tag <- roxy_test_tag("a {") + expect_parse_failure(tag_value(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + expect_parse_failure(tag_inherit(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + expect_parse_failure(tag_name(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + expect_parse_failure(tag_two_part(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + expect_parse_failure(tag_words(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + expect_parse_failure(tag_words_line(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + expect_parse_failure(tag_examples(tag)) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + # markdown tags return empty values + (expect_warning(tag_markdown(tag))) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + (expect_warning(tag_markdown_with_sections(tag))) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + +--- + + Code + markdown_on() + Output + [1] TRUE + Code + tag <- roxy_test_tag("{") + (expect_warning(tag_markdown(tag))) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + Code + tag <- roxy_test_tag("# one\ntwo\n# three\nfour {") + (expect_warning(tag_markdown_with_sections(tag))) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + diff --git a/tests/testthat/test-block.R b/tests/testthat/test-block.R index 25b216d6d..32677588e 100644 --- a/tests/testthat/test-block.R +++ b/tests/testthat/test-block.R @@ -237,33 +237,31 @@ test_that("evaluation occurs during parsing", { }) test_that("errors are propagated", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " foo <- function() stop('Uhoh') + #' @name foo #' @eval foo() NULL" - ), - "failed with error" + ) ) }) test_that("must return non-NA string", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " foo <- function() NA #' @eval foo() NULL" - ), - "did not evaluate to a string" + ) ) - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " foo <- function() NA_character_ #' @eval foo() NULL" - ), - "result contained NA" + ) ) }) diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index e2f9862e8..74647eaaa 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -175,10 +175,10 @@ test_that("other namespace tags produce correct output", { }) test_that("poorly formed importFrom throws error", { - expect_warning(roc_proc_text(namespace_roclet(), " + expect_snapshot_warning(roc_proc_text(namespace_roclet(), " #' @importFrom test NULL - "), "needs at least 2 words") + ")) }) test_that("multiline importFrom parsed correctly", { @@ -227,17 +227,16 @@ test_that("empty NAMESPACE generates zero-length vector", { test_that("rawNamespace must be valid code", { - expect_warning( + expect_snapshot_warning( roc_proc_text(namespace_roclet(), " - #' @rawNamespace if() { - #' @name a - NULL"), - "code failed to parse" + #' @rawNamespace a + + NULL") ) }) test_that("rawNamespace inserted unchanged", { out <- roc_proc_text(namespace_roclet(), " + #' @name a #' @rawNamespace xyz #' abc NULL") @@ -249,24 +248,22 @@ test_that("rawNamespace inserted unchanged", { # @evalNamespace ---------------------------------------------------------- test_that("evalNamespace generates warning when code is invalid", { - expect_warning( + expect_snapshot_warning( roc_proc_text(namespace_roclet(), " #' @evalNamespace a + #' @name a #' @title a - NULL"), - "code failed to parse" # Message generated by tag_code + NULL") ) }) test_that("evalNamespace generates warning when code raises error", { - expect_warning( + expect_snapshot_warning( roc_proc_text(namespace_roclet(), " - #' @evalNamespace stop('!') + #' @evalNamespace stop('Uhoh') #' @name a #' @title a - NULL"), - "failed with error" # From block_eval + NULL") ) }) diff --git a/tests/testthat/test-rd-inherit.R b/tests/testthat/test-rd-inherit.R index 2538467d6..e06c7e20b 100644 --- a/tests/testthat/test-rd-inherit.R +++ b/tests/testthat/test-rd-inherit.R @@ -62,12 +62,11 @@ test_that("relative links converted to absolute", { # tag parsing ------------------------------------------------------------- test_that("warns on unknown inherit type", { - expect_warning( + expect_snapshot_warning( parse_text(" #' @inherit fun blah NULL - "), - "Unknown inherit type: blah" + ") ) }) diff --git a/tests/testthat/test-rd-raw.R b/tests/testthat/test-rd-raw.R index 8d5f12444..382cad9af 100644 --- a/tests/testthat/test-rd-raw.R +++ b/tests/testthat/test-rd-raw.R @@ -10,24 +10,22 @@ test_that("rawRd inserted unchanged", { }) test_that("evalRd must be valid code", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " #' @evalRd a + #' @name a #' @title a - NULL"), - "code failed to parse" + NULL") ) }) test_that("error-ful evalRd generates warning", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " - #' @evalRd stop('!') + #' @evalRd stop('Uhoh') #' @name a #' @title a - NULL"), - "failed with error" + NULL") ) }) diff --git a/tests/testthat/test-tag-parser.R b/tests/testthat/test-tag-parser.R index 978f96d0b..02fb8e71c 100644 --- a/tests/testthat/test-tag-parser.R +++ b/tests/testthat/test-tag-parser.R @@ -1,18 +1,48 @@ test_that("tags containing only whitespace generate warning", { - tag <- roxy_tag("foo", " ") + expect_parse_failure <- function(code) { + (expect_warning(expect_null(code))) + } + + expect_snapshot({ + tag <- roxy_test_tag(" ") + expect_parse_failure(tag_value(tag)) + expect_parse_failure(tag_inherit(tag)) + expect_parse_failure(tag_name(tag)) + expect_parse_failure(tag_two_part(tag)) + expect_parse_failure(tag_name_description(tag)) + expect_parse_failure(tag_code(tag)) + expect_parse_failure(tag_examples(tag)) + expect_parse_failure(tag_markdown(tag)) + expect_parse_failure(tag_markdown_with_sections(tag)) + }) +}) +test_that("tags check for mismatched parents gives useful warnings", { expect_parse_failure <- function(code) { - expect_warning(out <- code, "requires") - expect_null(out) + (expect_warning(expect_null(code))) } - expect_parse_failure(tag_value(tag)) - expect_parse_failure(tag_inherit(tag)) - expect_parse_failure(tag_name(tag)) - expect_parse_failure(tag_two_part(tag)) - expect_parse_failure(tag_name_description(tag)) - expect_parse_failure(tag_code(tag)) - expect_parse_failure(tag_examples(tag)) - expect_parse_failure(tag_markdown(tag)) - expect_parse_failure(tag_markdown_with_sections(tag)) + expect_snapshot({ + tag <- roxy_test_tag("a {") + expect_parse_failure(tag_value(tag)) + expect_parse_failure(tag_inherit(tag)) + expect_parse_failure(tag_name(tag)) + expect_parse_failure(tag_two_part(tag)) + expect_parse_failure(tag_words(tag)) + expect_parse_failure(tag_words_line(tag)) + expect_parse_failure(tag_examples(tag)) + + "markdown tags return empty values" + (expect_warning(tag_markdown(tag))) + (expect_warning(tag_markdown_with_sections(tag))) + }) + + local_markdown() + expect_snapshot({ + markdown_on() + tag <- roxy_test_tag("{") + (expect_warning(tag_markdown(tag))) + tag <- roxy_test_tag("# one\ntwo\n# three\nfour {") + (expect_warning(tag_markdown_with_sections(tag))) + }) }) From 40166b7499dd6c3ce2bdc7f1928f97b30e1889ca Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 4 Apr 2022 17:45:59 -0500 Subject: [PATCH 02/18] Improve tag_inherits warning --- R/tag-parser.R | 2 +- tests/testthat/_snaps/rd-inherit.md | 2 +- tests/testthat/_snaps/tag-parser.md | 9 +++++++++ tests/testthat/test-tag-parser.R | 7 +++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/R/tag-parser.R b/R/tag-parser.R index cfeba3493..382b15d7b 100644 --- a/R/tag-parser.R +++ b/R/tag-parser.R @@ -50,7 +50,7 @@ tag_inherit <- function(x) { } else { unknown <- setdiff(fields, all) if (length(unknown) > 0) { - warn_roxy_tag(x, "inherits from unknown type {.str unknown}") + warn_roxy_tag(x, "attempts to inherit from unknown type {.str {unknown}}") fields <- intersect(fields, all) } } diff --git a/tests/testthat/_snaps/rd-inherit.md b/tests/testthat/_snaps/rd-inherit.md index 4f338a130..0b3799db7 100644 --- a/tests/testthat/_snaps/rd-inherit.md +++ b/tests/testthat/_snaps/rd-inherit.md @@ -7,7 +7,7 @@ # warns on unknown inherit type - [:2] @inherit inherits from unknown type "unknown" + [:2] @inherit attempts to inherit from unknown type "blah" # warns if can't find section diff --git a/tests/testthat/_snaps/tag-parser.md b/tests/testthat/_snaps/tag-parser.md index 4de62d2c6..e8bddbbf9 100644 --- a/tests/testthat/_snaps/tag-parser.md +++ b/tests/testthat/_snaps/tag-parser.md @@ -136,3 +136,12 @@ Warning: [test.R:1] @test has mismatched braces or quotes +# tag_inhert checks for valid inherits + + Code + tag <- roxy_test_tag("foo params sction") + . <- tag_inherit(tag) + Condition + Warning: + [test.R:1] @test attempts to inherit from unknown type "sction" + diff --git a/tests/testthat/test-tag-parser.R b/tests/testthat/test-tag-parser.R index 02fb8e71c..a1f12aefb 100644 --- a/tests/testthat/test-tag-parser.R +++ b/tests/testthat/test-tag-parser.R @@ -46,3 +46,10 @@ test_that("tags check for mismatched parents gives useful warnings", { (expect_warning(tag_markdown_with_sections(tag))) }) }) + +test_that("tag_inhert checks for valid inherits", { + expect_snapshot({ + tag <- roxy_test_tag("foo params sction") + . <- tag_inherit(tag) + }) +}) From 5a67c08d9da8a83df1cf0ea94ee29074a486e603 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 4 Apr 2022 17:56:34 -0500 Subject: [PATCH 03/18] Finish tests for tag parsers --- R/tag-parser.R | 26 ++++++---- tests/testthat/_snaps/namespace.md | 2 +- tests/testthat/_snaps/tag-parser.md | 79 ++++++++++++++++++++++++++++- tests/testthat/helper-test.R | 4 ++ tests/testthat/test-tag-parser.R | 55 +++++++++++++++++--- 5 files changed, 145 insertions(+), 21 deletions(-) diff --git a/R/tag-parser.R b/R/tag-parser.R index 382b15d7b..16b595109 100644 --- a/R/tag-parser.R +++ b/R/tag-parser.R @@ -68,17 +68,20 @@ tag_inherit <- function(x) { #' @rdname tag_parsers tag_name <- function(x) { if (str_trim(x$raw) == "") { - warn_roxy_tag(x, "requires a name") + warn_roxy_tag(x, "requires a value") NULL } else if (!rdComplete(x$raw, is_code = FALSE)) { warn_roxy_tag(x, "has mismatched braces or quotes") NULL - } else if (str_count(x$raw, "\\s+") > 1) { - warn_roxy_tag(x, "must have only one argument") - NULL } else { - x$val <- str_trim(x$raw) - x + n <- str_count(x$raw, "\\s+") + if (n > 1) { + warn_roxy_tag(x, "must have only one argument, not {n}") + NULL + } else { + x$val <- str_trim(x$raw) + x + } } } @@ -132,10 +135,10 @@ tag_words <- function(x, min = 0, max = Inf) { words <- str_split(str_trim(x$raw), "\\s+")[[1]] if (length(words) < min) { - warn_roxy_tag(x, "must have at least {min} words") + warn_roxy_tag(x, "must have at least {min} word{?s}, not {length(words)}") NULL } else if (length(words) > max) { - warn_roxy_tag(x, "must have at most {max} words") + warn_roxy_tag(x, "must have at most {max} word{?s}, not {length(words)}") NULL } else { x$val <- words @@ -148,8 +151,9 @@ tag_words <- function(x, min = 0, max = Inf) { tag_words_line <- function(x) { x$val <- str_trim(x$raw) - if (str_detect(x$val, "\n")) { - warn_roxy_tag(x, "may only span a single line") + n_lines <- str_count(x$val, "\n") + if (n_lines > 1) { + warn_roxy_tag(x, "must only span a single line, not {n_lines}") NULL } else if (!rdComplete(x$val, is_code = FALSE)) { warn_roxy_tag(x, "has mismatched braces or quotes") @@ -166,7 +170,7 @@ tag_toggle <- function(x) { x$val <- str_trim(x$raw) if (x$val != "") { - warn_roxy_tag(x, "has no parameters") + warn_roxy_tag(x, "must not be followed by any text") NULL } else { x diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index e033d2a5d..6e14f144a 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -1,6 +1,6 @@ # poorly formed importFrom throws error - [:2] @importFrom must have at least 2 words + [:2] @importFrom must have at least 2 words, not 1 # rawNamespace must be valid code diff --git a/tests/testthat/_snaps/tag-parser.md b/tests/testthat/_snaps/tag-parser.md index e8bddbbf9..d76f0fbca 100644 --- a/tests/testthat/_snaps/tag-parser.md +++ b/tests/testthat/_snaps/tag-parser.md @@ -18,7 +18,7 @@ Output Warning: - [test.R:1] @test requires a name + [test.R:1] @test requires a value Code expect_parse_failure(tag_two_part(tag)) Output @@ -145,3 +145,80 @@ Warning: [test.R:1] @test attempts to inherit from unknown type "sction" +# tag_name() checks for valid names + + Code + tag <- roxy_test_tag("a b c") + expect_parse_failure(tag_name(tag)) + Output + + Warning: + [test.R:1] @test must have only one argument, not 2 + +# tag_two_part() gives useful warnings + + Code + tag <- roxy_test_tag("a") + expect_parse_failure(tag_two_part(tag, "name", "value")) + Output + + Warning: + [test.R:1] @test requires name and value + Code + tag <- roxy_test_tag("{ }") + (expect_warning(tag_two_part(tag, "name", "value"))) + Output + + Warning: + [test.R:1] @test has mismatched braces or quotes + +# tag_words() gives useful warnings + + Code + tag <- roxy_test_tag("a b") + expect_parse_failure(tag_words(tag, 3, 3)) + Output + + Warning: + [test.R:1] @test must have at least 3 words, not 2 + Code + expect_parse_failure(tag_words(tag, 1, 1)) + Output + + Warning: + [test.R:1] @test must have at most 1 word, not 2 + +# tag_words_line() gives useful warnings + + Code + tag <- roxy_test_tag("a\nb\n2") + expect_parse_failure(tag_words_line(tag)) + Output + + Warning: + [test.R:1] @test must only span a single line, not 2 + +# tag_toggle() gives useful warnings + + Code + tag <- roxy_test_tag("x") + expect_parse_failure(tag_toggle(tag)) + Output + + Warning: + [test.R:1] @test must not be followed by any text + +# tag_code() gives useful warnings + + Code + tag <- roxy_test_tag("a + ") + expect_parse_failure(tag_code(tag)) + Output + + Warning: + [test.R:1] @test failed to parse + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: a + + ^ + diff --git a/tests/testthat/helper-test.R b/tests/testthat/helper-test.R index d61be2348..5d2e0973b 100644 --- a/tests/testthat/helper-test.R +++ b/tests/testthat/helper-test.R @@ -11,3 +11,7 @@ expect_equal_strings <- function(s1, s2, ignore_ws = TRUE) { } expect_equal(s1, s2) } + +expect_parse_failure <- function(code) { + (expect_warning(expect_null(code))) +} diff --git a/tests/testthat/test-tag-parser.R b/tests/testthat/test-tag-parser.R index a1f12aefb..e3b55eeb4 100644 --- a/tests/testthat/test-tag-parser.R +++ b/tests/testthat/test-tag-parser.R @@ -1,8 +1,4 @@ test_that("tags containing only whitespace generate warning", { - expect_parse_failure <- function(code) { - (expect_warning(expect_null(code))) - } - expect_snapshot({ tag <- roxy_test_tag(" ") expect_parse_failure(tag_value(tag)) @@ -18,10 +14,6 @@ test_that("tags containing only whitespace generate warning", { }) test_that("tags check for mismatched parents gives useful warnings", { - expect_parse_failure <- function(code) { - (expect_warning(expect_null(code))) - } - expect_snapshot({ tag <- roxy_test_tag("a {") expect_parse_failure(tag_value(tag)) @@ -53,3 +45,50 @@ test_that("tag_inhert checks for valid inherits", { . <- tag_inherit(tag) }) }) + +test_that("tag_name() checks for valid names", { + expect_snapshot({ + tag <- roxy_test_tag("a b c") + expect_parse_failure(tag_name(tag)) + }) +}) + +test_that("tag_two_part() gives useful warnings", { + local_markdown() + expect_snapshot({ + tag <- roxy_test_tag("a") + expect_parse_failure(tag_two_part(tag, "name", "value")) + + tag <- roxy_test_tag("{ }") + (expect_warning(tag_two_part(tag, "name", "value"))) + }) +}) + +test_that("tag_words() gives useful warnings", { + expect_snapshot({ + tag <- roxy_test_tag("a b") + expect_parse_failure(tag_words(tag, 3, 3)) + expect_parse_failure(tag_words(tag, 1, 1)) + }) +}) + +test_that("tag_words_line() gives useful warnings", { + expect_snapshot({ + tag <- roxy_test_tag("a\nb\n2") + expect_parse_failure(tag_words_line(tag)) + }) +}) + +test_that("tag_toggle() gives useful warnings", { + expect_snapshot({ + tag <- roxy_test_tag("x") + expect_parse_failure(tag_toggle(tag)) + }) +}) + +test_that("tag_code() gives useful warnings", { + expect_snapshot({ + tag <- roxy_test_tag("a + ") + expect_parse_failure(tag_code(tag)) + }) +}) From cd48ee86e3b0a99a8b293d2301f5d4dffb46c9bb Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 07:29:08 -0500 Subject: [PATCH 04/18] Convert and test `@describeIn` warnings --- R/rd-describe-in.R | 8 +++--- tests/testthat/_snaps/rd-describe-in.md | 12 ++++++++ tests/testthat/test-rd-describe-in.R | 37 +++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 tests/testthat/_snaps/rd-describe-in.md diff --git a/R/rd-describe-in.R b/R/rd-describe-in.R index 38058befb..ca68ea98b 100644 --- a/R/rd-describe-in.R +++ b/R/rd-describe-in.R @@ -10,15 +10,15 @@ topic_add_describe_in <- function(topic, block, env) { } if (is.null(block$object)) { - roxy_tag_warning(tag, "must be used with an object") + warn_roxy_tag(tag, "must be used with an object") return() } - if (block_has_tags(block, "name")) { - roxy_tag_warning(tag, "can not be used with @name") + if (block_has_tags(block, "name")) { + warn_roxy_tag(tag, "can not be used with @name") return() } if (block_has_tags(block, "rdname")) { - roxy_tag_warning(tag, "can not be used with @rdname") + warn_roxy_tag(tag, "can not be used with @rdname") return() } diff --git a/tests/testthat/_snaps/rd-describe-in.md b/tests/testthat/_snaps/rd-describe-in.md new file mode 100644 index 000000000..b0c5b4aa7 --- /dev/null +++ b/tests/testthat/_snaps/rd-describe-in.md @@ -0,0 +1,12 @@ +# complains about bad usage + + [:6] @describeIn must be used with an object + +--- + + [:6] @describeIn can not be used with @name + +--- + + [:6] @describeIn can not be used with @rdname + diff --git a/tests/testthat/test-rd-describe-in.R b/tests/testthat/test-rd-describe-in.R index aa7517478..ad471ac88 100644 --- a/tests/testthat/test-rd-describe-in.R +++ b/tests/testthat/test-rd-describe-in.R @@ -116,3 +116,40 @@ test_that("function names are escaped", { ")[[1]] expect_match(out$get_rd("minidesc"), "\\\\%foo\\\\%") }) + + +test_that("complains about bad usage", { + expect_snapshot_warning( + roc_proc_text(rd_roclet(), " + #' bar + bar <- 100 + + #' @name bar + #' @describeIn foo shortcut for foo + NULL + " + ) + ) + expect_snapshot_warning( + roc_proc_text(rd_roclet(), " + #' bar + bar <- 100 + + #' @name bar + #' @describeIn foo shortcut for foo + foo <- 10 + " + ) + ) + expect_snapshot_warning( + roc_proc_text(rd_roclet(), " + #' bar + bar <- 100 + + #' @rdname bar + #' @describeIn foo shortcut for foo + foo <- 10 + " + ) + ) +}) From 8543f3718aef875e4125e9b52e91dd5ef37c9698 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 08:30:09 -0500 Subject: [PATCH 05/18] Parse code once in tag_code() --- R/namespace.R | 2 +- R/rd-raw.R | 14 ++++++++------ R/tag-parser.R | 3 +-- tests/testthat/_snaps/block.md | 6 +++--- tests/testthat/_snaps/namespace.md | 10 +++++++--- tests/testthat/_snaps/rd-raw.md | 31 +++++++++++++++++++----------- tests/testthat/test-block.R | 1 + tests/testthat/test-namespace.R | 26 ++++--------------------- tests/testthat/test-rd-raw.R | 29 +++++++++------------------- 9 files changed, 54 insertions(+), 68 deletions(-) diff --git a/R/namespace.R b/R/namespace.R index 277c187e8..ab7a56db7 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -234,7 +234,7 @@ roxy_tag_parse.roxy_tag_rawNamespace <- function(x) { } #' @export roxy_tag_ns.roxy_tag_rawNamespace <- function(x, block, env, import_only = FALSE) { - x$val + x$raw } #' @export diff --git a/R/rd-raw.R b/R/rd-raw.R index 6dc49a45d..59691c444 100644 --- a/R/rd-raw.R +++ b/R/rd-raw.R @@ -16,19 +16,21 @@ format.rd_section_rawRd <- function(x, ...) { paste(x$value, collapse = "\n") } -roxy_tag_eval <- function(tag, env) { +roxy_tag_eval <- function(tag, env = new.env(emptyenv())) { tryCatch({ - expr <- parse(text = tag$val) - out <- eval(expr, envir = env) + out <- eval(tag$val, envir = env) if (!is.character(out)) { - roxy_tag_warning(tag, "did not evaluate to a string") + warn_roxy_tag(tag, "must evaluate to a character vector") + NULL } else if (anyNA(out)) { - roxy_tag_warning(tag, "result contained NA") + warn_roxy_tag(tag, "must not contain any missing values") + NULL } else { out } }, error = function(e) { - warn_roxy_tag(tag, "failed to execute", parent = e) + warn_roxy_tag(tag, "failed to evaluate", parent = e) + NULL }) } diff --git a/R/tag-parser.R b/R/tag-parser.R index 16b595109..b96516790 100644 --- a/R/tag-parser.R +++ b/R/tag-parser.R @@ -185,8 +185,7 @@ tag_code <- function(x) { NULL } else { tryCatch({ - parse(text = x$raw) - x$val <- x$raw + x$val <- parse(text = x$raw) x }, error = function(e) { warn_roxy_tag(x, "failed to parse", parent = e) diff --git a/tests/testthat/_snaps/block.md b/tests/testthat/_snaps/block.md index 199946b87..d9c0cdf68 100644 --- a/tests/testthat/_snaps/block.md +++ b/tests/testthat/_snaps/block.md @@ -15,15 +15,15 @@ # errors are propagated - [:4] @eval failed to execute + [:5] @eval failed to evaluate Caused by error in `foo()`: ! Uhoh # must return non-NA string - [:3] @eval did not evaluate to a string + [:3] @eval must evaluate to a character vector --- - [:3] @eval result contained NA + [:3] @eval must not contain any missing values diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index 6e14f144a..acd05a400 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -10,7 +10,7 @@ 1: a + ^ -# evalNamespace generates warning when code is invalid +# evalNamespace warns for bad code [:2] @evalNamespace failed to parse Caused by error in `parse()`: @@ -18,9 +18,13 @@ 1: a + ^ -# evalNamespace generates warning when code raises error +--- - [:2] @evalNamespace failed to execute + [:2] @evalNamespace failed to evaluate Caused by error: ! Uhoh +--- + + [:2] @evalNamespace must evaluate to a character vector + diff --git a/tests/testthat/_snaps/rd-raw.md b/tests/testthat/_snaps/rd-raw.md index f08515e82..6e690c2b7 100644 --- a/tests/testthat/_snaps/rd-raw.md +++ b/tests/testthat/_snaps/rd-raw.md @@ -1,14 +1,23 @@ -# evalRd must be valid code - - [:2] @evalRd failed to parse - Caused by error in `parse()`: - ! :2:0: unexpected end of input - 1: a + - ^ - # error-ful evalRd generates warning - [:2] @evalRd failed to execute - Caused by error: - ! Uhoh + Code + expect_parse_failure(roxy_tag_eval(roxy_test_tag(val = 1))) + Output + + Warning: + [test.R:1] @test must evaluate to a character vector + Code + expect_parse_failure(roxy_tag_eval(roxy_test_tag(val = NA_character_))) + Output + + Warning: + [test.R:1] @test must not contain any missing values + Code + expect_parse_failure(roxy_tag_eval(roxy_test_tag(val = quote(stop("Uhoh"))))) + Output + + Warning: + [test.R:1] @test failed to evaluate + Caused by error: + ! Uhoh diff --git a/tests/testthat/test-block.R b/tests/testthat/test-block.R index 32677588e..b899f9048 100644 --- a/tests/testthat/test-block.R +++ b/tests/testthat/test-block.R @@ -240,6 +240,7 @@ test_that("errors are propagated", { expect_snapshot_warning( roc_proc_text(rd_roclet(), " foo <- function() stop('Uhoh') + #' Title #' @name foo #' @eval foo() NULL" diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index 74647eaaa..5e8471b05 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -247,7 +247,7 @@ test_that("rawNamespace inserted unchanged", { # @evalNamespace ---------------------------------------------------------- -test_that("evalNamespace generates warning when code is invalid", { +test_that("evalNamespace warns for bad code", { expect_snapshot_warning( roc_proc_text(namespace_roclet(), " #' @evalNamespace a + @@ -255,9 +255,7 @@ test_that("evalNamespace generates warning when code is invalid", { #' @title a NULL") ) -}) -test_that("evalNamespace generates warning when code raises error", { expect_snapshot_warning( roc_proc_text(namespace_roclet(), " #' @evalNamespace stop('Uhoh') @@ -265,29 +263,13 @@ test_that("evalNamespace generates warning when code raises error", { #' @title a NULL") ) -}) -test_that("evalNamespace generates warning when code doesn't eval to string", { - # Not character - expect_warning( - roc_proc_text(namespace_roclet(), " - z <- 10 - #' @evalNamespace z * 2 - #' @name a - #' @title a - NULL"), - "did not evaluate to a string" # From block_eval - ) - - # NA_character_ not allowed - expect_warning( + expect_snapshot_warning( roc_proc_text(namespace_roclet(), " - nms <- NA_character_ - #' @evalNamespace nms + #' @evalNamespace 1 #' @name a #' @title a - NULL"), - "result contained NA" # From block_eval + NULL") ) }) diff --git a/tests/testthat/test-rd-raw.R b/tests/testthat/test-rd-raw.R index 382cad9af..6d4a5d3bd 100644 --- a/tests/testthat/test-rd-raw.R +++ b/tests/testthat/test-rd-raw.R @@ -9,26 +9,6 @@ test_that("rawRd inserted unchanged", { expect_equal(lines[[9]], "#this is a comment") }) -test_that("evalRd must be valid code", { - expect_snapshot_warning( - roc_proc_text(rd_roclet(), " - #' @evalRd a + - #' @name a - #' @title a - NULL") - ) -}) - -test_that("error-ful evalRd generates warning", { - expect_snapshot_warning( - roc_proc_text(rd_roclet(), " - #' @evalRd stop('Uhoh') - #' @name a - #' @title a - NULL") - ) -}) - test_that("evalRd inserted unchanged", { out <- roc_proc_text(rd_roclet(), " z <- 10 @@ -39,3 +19,12 @@ test_that("evalRd inserted unchanged", { expect_equal(out$get_value("rawRd"), "20") }) + +test_that("error-ful evalRd generates warning", { + expect_snapshot({ + expect_parse_failure(roxy_tag_eval(roxy_test_tag(val = 1))) + expect_parse_failure(roxy_tag_eval(roxy_test_tag(val = NA_character_))) + expect_parse_failure(roxy_tag_eval(roxy_test_tag(val = quote(stop('Uhoh'))))) + }) +}) + From bf1ababfdcdccdede6e106ccc5a7b9129c1542b4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 08:33:32 -0500 Subject: [PATCH 06/18] Update markdown link warning --- R/markdown-link.R | 7 ++++--- tests/testthat/_snaps/markdown-link.md | 8 +++++--- tests/testthat/test-markdown-link.R | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/R/markdown-link.R b/R/markdown-link.R index cd4cb1890..04f66f00b 100644 --- a/R/markdown-link.R +++ b/R/markdown-link.R @@ -105,9 +105,10 @@ parse_link <- function(destination, contents, state) { if (!all(xml_name(contents) %in% c("text", "softbreak", "linebreak"))) { incorrect <- setdiff(unique(xml_name(contents)), c("text", "softbreak", "linebreak")) - roxy_tag_warning(state$tag, - "Links must contain plain text. Problematic link: ", destination - ) + warn_roxy_tag(state$tag, c( + "markdown: links must contain plain text", + "Problematic link: {destination}" + )) return("") } diff --git a/tests/testthat/_snaps/markdown-link.md b/tests/testthat/_snaps/markdown-link.md index 0be10a458..1561fdb64 100644 --- a/tests/testthat/_snaps/markdown-link.md +++ b/tests/testthat/_snaps/markdown-link.md @@ -4,14 +4,16 @@ markdown("[`foo` bar][x]", tag = tag) Condition Warning: - [foo.R:10] @title Links must contain plain text. Problematic link: x + [foo.R:10] @title markdown: links must contain plain text + Problematic link: x Output [1] "" Code - markdown("[`foo{}` bar __baz__][x]", tag = tag) + markdown("[__baz__][x]", tag = tag) Condition Warning: - [foo.R:10] @title Links must contain plain text. Problematic link: x + [foo.R:10] @title markdown: links must contain plain text + Problematic link: x Output [1] "" diff --git a/tests/testthat/test-markdown-link.R b/tests/testthat/test-markdown-link.R index e9fc3313a..f6297a5b2 100644 --- a/tests/testthat/test-markdown-link.R +++ b/tests/testthat/test-markdown-link.R @@ -67,7 +67,7 @@ test_that("non-text nodes in links fails", { expect_snapshot({ markdown("[`foo` bar][x]", tag = tag) - markdown("[`foo{}` bar __baz__][x]", tag = tag) + markdown("[__baz__][x]", tag = tag) }) }) From 1675f4f1c4436fbbc1cc9e40dd044f9f8b6115ac Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 08:41:40 -0500 Subject: [PATCH 07/18] Update namespace.R --- R/namespace.R | 4 +--- tests/testthat/_snaps/namespace.md | 4 ++++ tests/testthat/test-namespace.R | 5 ++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/R/namespace.R b/R/namespace.R index ab7a56db7..596f582cc 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -175,9 +175,7 @@ roxy_tag_ns.roxy_tag_exportS3Method <- function(x, block, env, import_only = FAL obj <- block$object if (length(x$val) < 2 && !inherits(obj, "s3method")) { - roxy_tag_warning(x, - "`@exportS3Method` and `@exportS3Method generic` must be used with an S3 method" - ) + warn_roxy_tag(x, "must be used with an S3 method when it has 0 or 1 values") return() } diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index acd05a400..19ac60e50 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -1,3 +1,7 @@ +# @exportS3method generatedsS3method() + + [:1] @exportS3Method must be used with an S3 method when it has 0 or 1 values + # poorly formed importFrom throws error [:2] @importFrom must have at least 2 words, not 1 diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index 5e8471b05..9f9adf42e 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -88,12 +88,11 @@ test_that("@exportS3method generatedsS3method()", { ") expect_equal(out, "S3method(base::mean,foo)") - expect_warning( + expect_snapshot_warning( roc_proc_text(namespace_roclet(), "#' @exportS3Method base::mean NULL - "), - "an S3 method" + ") ) out <- roc_proc_text(namespace_roclet(), From beefa64f9244492989cc79906e99ddd33dff11c3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 08:43:02 -0500 Subject: [PATCH 08/18] Update rd-include-rmd --- R/rd-include-rmd.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/rd-include-rmd.R b/R/rd-include-rmd.R index 256f62ee5..cf04c713a 100644 --- a/R/rd-include-rmd.R +++ b/R/rd-include-rmd.R @@ -1,7 +1,7 @@ #' @export roxy_tag_parse.roxy_tag_includeRmd <- function(x) { if (!is_installed("rmarkdown")) { - roxy_tag_warning(x, "Needs the rmarkdown package") + warn_roxy_tag(x, "requires the rmarkdown package") return() } From 5b94b810b5c2678f8413819009e036efefe73136 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:05:19 -0500 Subject: [PATCH 09/18] Update rd warnings; add new warn_roxy_block --- R/block.R | 7 +++- R/rd.R | 11 +++--- tests/testthat/_snaps/block.md | 4 +++ tests/testthat/_snaps/rd.md | 14 ++++++++ tests/testthat/test-block.R | 14 ++++++++ tests/testthat/test-object-defaults.R | 19 ++++++---- tests/testthat/test-object-import.R | 13 ------- tests/testthat/test-rd.R | 52 +++++++++++++++++---------- 8 files changed, 91 insertions(+), 43 deletions(-) create mode 100644 tests/testthat/_snaps/rd.md diff --git a/R/block.R b/R/block.R index ca84dc77f..e71858d41 100644 --- a/R/block.R +++ b/R/block.R @@ -181,7 +181,7 @@ block_get_tag <- function(block, tag) { } else if (n == 1) { block$tags[[matches]] } else { - roxy_tag_warning(block$tags[[matches[[2]]]], "May only use one @", tag, " per block") + warn_roxy_block(block, "Block must contain only one @{tag}") block$tags[[matches[[1]]]] } } @@ -289,3 +289,8 @@ parse_description <- function(tags) { c(compact(list(title, description, details)), tags) } + +warn_roxy_block <- function(block, message, ...) { + message[[1]] <- paste0("[", block$file, ":", block$line, "] ", message[[1]]) + cli::cli_warn(message, ..., .envir = parent.frame()) +} diff --git a/R/rd.R b/R/rd.R index 462a1e9a2..22b3caad3 100644 --- a/R/rd.R +++ b/R/rd.R @@ -121,7 +121,10 @@ block_to_rd.roxy_block <- function(block, base_path, env) { name <- block_get_tag(block, "name")$val %||% block$object$topic if (is.null(name)) { - roxy_tag_warning(block$tags[[1]], "Missing name") + warn_roxy_block(block, c( + "Block must have a @name", + i = "Either document an existing object or manually specify with @name" + )) return() } @@ -132,7 +135,7 @@ block_to_rd.roxy_block <- function(block, base_path, env) { } if (rd$has_section("description") && rd$has_section("reexport")) { - roxy_tag_warning(block$tags[[1]], "Can't use description when re-exporting") + warn_roxy_block(block, "Block must not include a description when re-exporting a function") return() } @@ -159,7 +162,7 @@ block_to_rd.roxy_block_r6class <- function(block, base_path, env) { name <- block_get_tag(block, "name")$val %||% block$object$topic if (is.null(name)) { - roxy_tag_warning(block$tags[[1]], "Missing name") + warn_roxy_block(block, "must have a @name") return() } @@ -170,7 +173,7 @@ block_to_rd.roxy_block_r6class <- function(block, base_path, env) { rd$add(roxy_tag_rd(block_get_tag(block, "title"), env = env, base_path = base_path)) if (rd$has_section("description") && rd$has_section("reexport")) { - roxy_tag_warning(block$tags[[1]], "Can't use description when re-exporting") + warn_roxy_block(block, "Block must not include a description when re-exporting a function") return() } diff --git a/tests/testthat/_snaps/block.md b/tests/testthat/_snaps/block.md index d9c0cdf68..a5c13c17e 100644 --- a/tests/testthat/_snaps/block.md +++ b/tests/testthat/_snaps/block.md @@ -27,3 +27,7 @@ [:3] @eval must not contain any missing values +# warns about duplicate tags + + [:5] Block must contain only one @rdname + diff --git a/tests/testthat/_snaps/rd.md b/tests/testthat/_snaps/rd.md new file mode 100644 index 000000000..76db9eb0b --- /dev/null +++ b/tests/testthat/_snaps/rd.md @@ -0,0 +1,14 @@ +# documenting unknown function requires name + + [:6] Block must have a @name + i Either document an existing object or manually specify with @name + +# can't set description and re-export + + [:4] Block must not include a description when re-exporting a function + +# documenting NA gives useful error message (#194) + + [:3] Block must have a @name + i Either document an existing object or manually specify with @name + diff --git a/tests/testthat/test-block.R b/tests/testthat/test-block.R index b899f9048..10279de60 100644 --- a/tests/testthat/test-block.R +++ b/tests/testthat/test-block.R @@ -277,3 +277,17 @@ test_that("also works with namespace roclet", { expect_equal(out, "export(a)") }) + +# other ------------------------------------------------------------------- + + +test_that("warns about duplicate tags", { + expect_snapshot_warning({ + roc_proc_text(rd_roclet(), " + #' Foo + #' @rdname foo + #' @rdname bar + foo <- function() {} + ") + }) +}) diff --git a/tests/testthat/test-object-defaults.R b/tests/testthat/test-object-defaults.R index a89d09a6a..1729afb74 100644 --- a/tests/testthat/test-object-defaults.R +++ b/tests/testthat/test-object-defaults.R @@ -47,13 +47,18 @@ test_that("@docType class automatically added to reference class objects", { test_that("can create package documentation", { mockr::with_mock( - read.description = function(...) - list(Package = "roxygen_devtest", - Title = "Package Title", - Description = "Package description."), - out <- roc_proc_text(rd_roclet(), " - #' @details Details. - '_PACKAGE'")[[1]] + read.description = function(...) { + list( + Package = "roxygen_devtest", + Title = "Package Title", + Description = "Package description." + ) + }, + { + out <- roc_proc_text(rd_roclet(), " + #' @details Details. + '_PACKAGE'")[[1]] + } ) expect_equal(out$get_value("name"), "roxygen_devtest-package") expect_equal(out$get_value("alias"), c("roxygen_devtest", "roxygen_devtest-package")) diff --git a/tests/testthat/test-object-import.R b/tests/testthat/test-object-import.R index 17e137e80..a9f266995 100644 --- a/tests/testthat/test-object-import.R +++ b/tests/testthat/test-object-import.R @@ -39,19 +39,6 @@ test_that("description generated correctly", { expect_null(out$get_section("description")) }) -test_that("can't set description and re-export", { - expect_warning( - out <- roc_proc_text(rd_roclet(), " - #' @description NOPE - #' @export - magrittr::`%>%` - "), - "Can't use description when re-exporting" - ) - - expect_length(out, 0) -}) - test_that("warnings for unknown packages and objects", { expect_warning( format(rd_section_reexport("11papaya", "fun")), diff --git a/tests/testthat/test-rd.R b/tests/testthat/test-rd.R index 147a3883b..45522caf4 100644 --- a/tests/testthat/test-rd.R +++ b/tests/testthat/test-rd.R @@ -37,25 +37,36 @@ test_that("deleted objects not documented", { }) test_that("documenting unknown function requires name", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " #' Virtual Class To Enforce Max Slot Length setClass('A') #' Validity function. setValidity('A', function(object) TRUE)" - ), - "Missing name" + ) ) }) +test_that("can't set description and re-export", { + expect_snapshot_warning( + out <- roc_proc_text(rd_roclet(), " + #' @description NOPE + #' @export + magrittr::`%>%` + ") + ) + + expect_length(out, 0) +}) + + test_that("documenting NA gives useful error message (#194)", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " #' Missing value NA" - ), - "Missing name" + ) ) }) @@ -91,18 +102,23 @@ test_that("@description NULL", { # But drop for package docs mockr::with_mock( - read.description = function(...) - list(Package = "roxygen_devtest", - Title = "Package Title", - Description = "Package description."), - out <- roc_proc_text(rd_roclet(), " - #' Title - #' - #' @docType package - #' @description NULL - #' @name pkg - '_PACKAGE' - ") + read.description = function(...) { + list( + Package = "roxygen_devtest", + Title = "Package Title", + Description = "Package description." + ) + }, + { + out <- roc_proc_text(rd_roclet(), " + #' Title + #' + #' @docType package + #' @description NULL + #' @name pkg + '_PACKAGE' + ") + } ) expect_null(out[[1]]$get_value("description")) }) From 67cc370aa93b0b9d7360dba2b8778e51fb5ad25e Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:13:29 -0500 Subject: [PATCH 10/18] Tweak md/noMd conflict warning + tests --- R/markdown-state.R | 13 ++++--- tests/testthat/_snaps/markdown-state.md | 4 +++ tests/testthat/test-markdown-state.R | 45 +++++++++---------------- 3 files changed, 27 insertions(+), 35 deletions(-) create mode 100644 tests/testthat/_snaps/markdown-state.md diff --git a/R/markdown-state.R b/R/markdown-state.R index 975f4ecd5..88e2dff9f 100644 --- a/R/markdown-state.R +++ b/R/markdown-state.R @@ -30,12 +30,15 @@ markdown_activate <- function(tags) { has_nomd <- "noMd" %in% names if (has_md && has_nomd) { - roxy_tag_warning(tags[[1]], "Both @md and @noMd, no markdown parsing") + md_tag <- tags[names == "md"][[1]] + warn_roxy_tag(md_tag, "conflicts with @noMd; turning markdown parsing off") + + md <- FALSE + } else { + md <- roxy_meta_get("markdown", FALSE) + if (has_md) md <- TRUE + if (has_nomd) md <- FALSE } - md <- roxy_meta_get("markdown", FALSE) - if (has_md) md <- TRUE - if (has_nomd) md <- FALSE - markdown_on(md) } diff --git a/tests/testthat/_snaps/markdown-state.md b/tests/testthat/_snaps/markdown-state.md new file mode 100644 index 000000000..def42053d --- /dev/null +++ b/tests/testthat/_snaps/markdown-state.md @@ -0,0 +1,4 @@ +# warning for both @md and @noMd + + [:5] @md conflicts with @noMd; turning markdown parsing off + diff --git a/tests/testthat/test-markdown-state.R b/tests/testthat/test-markdown-state.R index 6076f34c2..2d60f9fb3 100644 --- a/tests/testthat/test-markdown-state.R +++ b/tests/testthat/test-markdown-state.R @@ -85,35 +85,20 @@ test_that("turning on/off markdown locally", { }) test_that("warning for both @md and @noMd", { - expect_warning( - out1 <- roc_proc_text(rd_roclet(), " - #' Title - #' - #' Description with some `code` included. `More code.` - #' @md - #' @noMd - foo <- function() {}")[[1]], - "Both @md and @noMd, no markdown parsing" - ) - expect_equal( - out1$get_value("description"), - "Description with some `code` included. `More code.`" - ) + block <- " + #' Title + #' + #' `code` + #' @md + #' @noMd + foo <- function() {} + " - old <- roxy_meta_set("markdown", TRUE) - on.exit(roxy_meta_set("markdown", old)) - expect_warning( - out1 <- roc_proc_text(rd_roclet(), " - #' Title - #' - #' Description with some `code` included. `More code.` - #' @md - #' @noMd - foo <- function() {}")[[1]], - "Both @md and @noMd, no markdown parsing" - ) - expect_equal( - out1$get_value("description"), - "Description with some `code` included. `More code.`" - ) + expect_snapshot_warning(out1 <- roc_proc_text(rd_roclet(), block)) + expect_equal(out1[[1]]$get_value("description"), "`code`") + + # No translation even if markdown on generally + local_markdown() + suppressWarnings(out2 <- roc_proc_text(rd_roclet(), block)) + expect_equal(out2[[1]]$get_value("description"), "`code`") }) From 64468599caa7603475670e749fbd80471802ab5b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:15:23 -0500 Subject: [PATCH 11/18] Tweak `@section` warning --- R/rd-section.R | 6 +++--- tests/testthat/_snaps/rd-section.md | 5 +++++ tests/testthat/test-rd-section.R | 5 ++--- 3 files changed, 10 insertions(+), 6 deletions(-) create mode 100644 tests/testthat/_snaps/rd-section.md diff --git a/R/rd-section.R b/R/rd-section.R index 604e9fe5f..663d8b2ff 100644 --- a/R/rd-section.R +++ b/R/rd-section.R @@ -9,9 +9,9 @@ roxy_tag_rd.roxy_tag_section <- function(x, base_path, env) { title <- str_split(pieces[1], "\n")[[1]] if (length(title) > 1) { - roxy_tag_warning(x, paste0( - "Section title spans multiple lines.\n", - "Did you forget a colon (:) at the end of the title?" + warn_roxy_tag(x, c( + "title spans multiple lines.", + i = "Did you forget a colon (:) at the end of the title?" )) return() } diff --git a/tests/testthat/_snaps/rd-section.md b/tests/testthat/_snaps/rd-section.md new file mode 100644 index 000000000..3db07a8d3 --- /dev/null +++ b/tests/testthat/_snaps/rd-section.md @@ -0,0 +1,5 @@ +# warn if forgotton colom + + [:4] @section title spans multiple lines. + i Did you forget a colon (:) at the end of the title? + diff --git a/tests/testthat/test-rd-section.R b/tests/testthat/test-rd-section.R index 3c1a23a72..13f4446ca 100644 --- a/tests/testthat/test-rd-section.R +++ b/tests/testthat/test-rd-section.R @@ -1,5 +1,5 @@ test_that("warn if forgotton colom", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " #' Foo #' @@ -7,8 +7,7 @@ test_that("warn if forgotton colom", { #' Here. #' There foo <- function(x = '%') x - "), - "Section title spans multiple lines" + ") ) }) From 53a3534483fd845d385e963e2a3b90d3402bc4e8 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:20:31 -0500 Subject: [PATCH 12/18] Update `@example`(s) warnings --- R/rd-examples.R | 9 ++++++--- tests/testthat/_snaps/rd-examples.md | 15 ++++++++++++++- tests/testthat/test-rd-examples.R | 11 ++++------- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/R/rd-examples.R b/R/rd-examples.R index 609f768fa..b6aa70752 100644 --- a/R/rd-examples.R +++ b/R/rd-examples.R @@ -10,7 +10,7 @@ roxy_tag_parse.roxy_tag_examplesIf <- function(x) { tryCatch( suppressWarnings(parse(text = condition)), error = function(err) { - roxy_tag_warning(x, "failed to parse condition") + warn_roxy_tag(x, "condition failed to parse", parent = err) } ) @@ -33,7 +33,10 @@ roxy_tag_parse.roxy_tag_example <- function(x) { nl <- str_count(x$val, "\n") if (any(nl) > 0) { - roxy_tag_warning(x, "spans multiple lines. Do you want @examples?") + warn_roxy_tag(x, c( + "must be a single line", + i = "Do you want @examples?" + )) return() } @@ -52,7 +55,7 @@ roxy_tag_rd.roxy_tag_examplesIf <- function(x, base_path, env) { roxy_tag_rd.roxy_tag_example <- function(x, base_path, env) { path <- file.path(base_path, x$val) if (!file.exists(path)) { - roxy_tag_warning(x, "'", path, "' doesn't exist") + warn_roxy_tag(x, "{.path {path}} doesn't exist") return() } diff --git a/tests/testthat/_snaps/rd-examples.md b/tests/testthat/_snaps/rd-examples.md index 05a1b3684..1fb95af2c 100644 --- a/tests/testthat/_snaps/rd-examples.md +++ b/tests/testthat/_snaps/rd-examples.md @@ -6,6 +6,15 @@ example <- 'example2' } +# @example gives warning if used instead of @examples + + [:4] @example must be a single line + i Do you want @examples? + +# warns if path doesn't exist + + [:4] @example './this-path-doesnt-exist.R' doesn't exist + # @examplesIf \examples{ @@ -19,5 +28,9 @@ # @examplesIf warns about unparseable condition - [:4] @examplesIf failed to parse condition + [:4] @examplesIf condition failed to parse + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: 1 + + ^ diff --git a/tests/testthat/test-rd-examples.R b/tests/testthat/test-rd-examples.R index a485d33bd..21fa95ca4 100644 --- a/tests/testthat/test-rd-examples.R +++ b/tests/testthat/test-rd-examples.R @@ -49,29 +49,26 @@ test_that("@example does not introduce extra empty lines", { }) test_that("@example gives warning if used instead of @examples", { - expect_warning( + expect_snapshot_warning( out <- roc_proc_text(rd_roclet(), " #' @name a #' @title a #' @example #' a <- 1 #' a + b - NULL")[[1]], - "spans multiple lines" + NULL")[[1]] ) - expect_null(out$get_value("examples")) }) test_that("warns if path doesn't exist", { - expect_warning( + expect_snapshot_warning( roc_proc_text(rd_roclet(), " #' @name a #' @title a #' @example this-path-doesnt-exist.R NULL - "), - "doesn't exist" + ") ) }) From 9301823d40ee26f1eea81b73edc4e88cf3ec9a1a Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:26:39 -0500 Subject: [PATCH 13/18] Update markdown warnings --- R/markdown.R | 17 +++++++++++++---- tests/testthat/_snaps/markdown.md | 5 +++++ tests/testthat/test-markdown.R | 2 +- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/R/markdown.R b/R/markdown.R index 931c6950a..80e6af67a 100644 --- a/R/markdown.R +++ b/R/markdown.R @@ -183,7 +183,10 @@ mdxml_children_to_rd <- function(xml, state) { mdxml_node_to_rd <- function(xml, state) { if (!inherits(xml, "xml_node") || ! xml_type(xml) %in% c("text", "element")) { - roxy_tag_warning(state$tag, "Internal markdown translation failure") + warn_roxy_tag(state$tag, c( + "markdown translation failed", + "internal error" + )) return("") } @@ -221,11 +224,17 @@ mdxml_node_to_rd <- function(xml, state) { } mdxml_unknown <- function(xml, tag) { - roxy_tag_warning(tag, "Unknown xml node: ", xml_name(xml)) + warn_roxy_tag(tag, c( + "markdown translation failed", + x = "Internal error: unknown xml node {xml_name(xml)}" + )) escape_comment(xml_text(xml)) } mdxml_unsupported <- function(xml, tag, feature) { - roxy_tag_warning(tag, "Use of ", feature, " is not currently supported") + warn_roxy_tag(tag, c( + "markdown translation failed", + x = "{feature} are not currently supported" + )) escape_comment(xml_text(xml)) } @@ -398,7 +407,7 @@ mdxml_html_block <- function(xml, state) { mdxml_html_inline <- function(xml, state) { if (state$tag$tag != "includeRmd") { - return(mdxml_unsupported(xml, state$tag, "inline HTML")) + return(mdxml_unsupported(xml, state$tag, "inline HTML components")) } paste0( "\\if{html}{\\out{", diff --git a/tests/testthat/_snaps/markdown.md b/tests/testthat/_snaps/markdown.md index 5e01b8aec..d906f493e 100644 --- a/tests/testthat/_snaps/markdown.md +++ b/tests/testthat/_snaps/markdown.md @@ -34,3 +34,8 @@ } +# unhandled markdown generates warning + + [:4] @description markdown translation failed + x block quotes are not currently supported + diff --git a/tests/testthat/test-markdown.R b/tests/testthat/test-markdown.R index f43057ee8..bfd353024 100644 --- a/tests/testthat/test-markdown.R +++ b/tests/testthat/test-markdown.R @@ -461,7 +461,7 @@ test_that("unhandled markdown generates warning", { #' @name x NULL " - expect_warning(roc_proc_text(rd_roclet(), text), "block quotes") + expect_snapshot_warning(roc_proc_text(rd_roclet(), text)) }) test_that("level 1 heading in markdown generates warning in some tags", { From a2030504d629dbc8073c6cf5e3126d049227b646 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:42:04 -0500 Subject: [PATCH 14/18] Update R6 warnings --- R/rd-r6.R | 51 +++++++++++++++++++--------------- tests/testthat/_snaps/rd-r6.md | 48 ++++++++++++++++++++++++++------ tests/testthat/test-rd-r6.R | 23 +++++---------- 3 files changed, 74 insertions(+), 48 deletions(-) diff --git a/R/rd-r6.R b/R/rd-r6.R index 1ae57a474..6e74c0d06 100644 --- a/R/rd-r6.R +++ b/R/rd-r6.R @@ -18,7 +18,7 @@ topic_add_r6_methods <- function(rd, block, env) { del <- c(del, i) meth <- find_method_for_tag(methods, tag) if (is.na(meth)) { - roxy_tag_warning(tag, "Cannot find matching R6 method") + warn_roxy_tag(tag, "Cannot find matching R6 method") next } midx <- which(meth == methods$name) @@ -29,7 +29,9 @@ topic_add_r6_methods <- function(rd, block, env) { methods <- add_default_methods(methods) nodoc <- map_int(methods$tags, length) == 0 - r6_warning(block, "undocumented R6 method[s]: %s", methods$name[nodoc]) + if (any(nodoc)) { + warn_roxy_block(block, "Undocumented R6 method[s]: {methods$name[nodoc]}") + } block$tags[del] <- NULL @@ -116,15 +118,21 @@ r6_fields <- function(block, r6data) { # Check for missing fields miss <- setdiff(fields, docd) - r6_warning(block, "undocumented R6 field[s]: %s", miss) + if (length(miss) > 0) { + warn_roxy_block(block, "Undocumented R6 field[s]: {miss}") + } # Check for duplicate fields dup <- unique(docd[duplicated(docd)]) - r6_warning(block, "R6 field[s] documented multiple times: %s", dup) + if (length(dup) > 0) { + warn_roxy_block(block, "R6 field[s] documented multiple times: {dup}") + } # Check for extra fields xtra <- setdiff(docd, fields) - r6_warning(block, "unknown R6 field[s]: %s", xtra) + if (length(xtra) > 0) { + warn_roxy_block(block, "Unknown R6 field[s]: {xtra}") + } if (length(docd) == 0) return() @@ -156,11 +164,15 @@ r6_active_bindings <- function(block, r6data) { # Check for missing bindings miss <- setdiff(active, docd) - r6_warning(block, "undocumented R6 active binding[s]: %s", miss) + if (length(miss) > 0) { + warn_roxy_block(block, "Undocumented R6 active binding[s]: {miss}") + } # Check for duplicate bindings dup <- unique(docd[duplicated(docd)]) - r6_warning(block, "R6 active binding[s] documented multiple times: %s", dup) + if (length(dup) > 0) { + warn_roxy_block(block, "R6 active binding[s] documented multiple times: {dup}") + } if (length(docd) == 0) return() @@ -334,10 +346,10 @@ r6_method_params <- function(block, method) { mnames <- str_trim(unlist(strsplit(nms, ","))) dup <- unique(mnames[duplicated(mnames)]) for (m in dup) { - roxy_warning( - sprintf("argument `%s` documented multiple times for R6 method `%s`", m, method$name), - file = block$file, line = method$line - ) + warn_roxy_block(block, c( + "Must use one @param for each argument", + x = "${method$name}({m}) is documented multiple times" + )) } # Now add the missing ones from the class @@ -357,10 +369,10 @@ r6_method_params <- function(block, method) { mnames <- str_trim(unlist(strsplit(nms, ","))) miss <- setdiff(fnames, mnames) for (m in miss) { - roxy_warning( - sprintf("argument `%s` undocumented for R6 method `%s()`", m, method$name), - file = block$file, line = method$line - ) + warn_roxy_block(block, c( + "Must use one @param for each argument", + x = "${method$name}({m}) is not documented" + )) } if (length(par) == 0) return() @@ -390,7 +402,7 @@ r6_method_return <- function(block, method) { ret <- purrr::keep(method$tags[[1]], function(t) t$tag == "return") if (length(ret) == 0) return() if (length(ret) > 1) { - roxy_tag_warning(ret[[2]], "May only use one @return per R6 method") + warn_roxy_block(block, "Must use one @return per R6 method") } ret <- ret[[1]] c( @@ -445,10 +457,3 @@ first_five <- function(x) { if (length(x) > 5) x <- c(x[1:5], "...") paste(x, collapse = ", ") } - -r6_warning <- function(block, template, bad) { - if (length(bad) == 0) return() - badlist <- first_five(bad) - template <- gsub("[s]", if (length(bad) == 1) "" else "s", template, fixed = TRUE) - roxy_warning(sprintf(template, badlist), file = block$file, line = block$line) -} diff --git a/tests/testthat/_snaps/rd-r6.md b/tests/testthat/_snaps/rd-r6.md index 6474de6dd..a40d71a16 100644 --- a/tests/testthat/_snaps/rd-r6.md +++ b/tests/testthat/_snaps/rd-r6.md @@ -1,12 +1,42 @@ +# R6 edge cases, class without (documented) fields + + Code + topic_add_r6_methods(rd, block, environment()) + Condition + Warning: + [:7] Undocumented R6 field[s]: undocumented_field + +# warning if no method comes after the docs + + Code + topic_add_r6_methods(rd, block, environment()) + Condition + Warning: + [:10] @description Cannot find matching R6 method + # integration test - [1] "[roxygen-block-3.R:35] @return May only use one @return per R6 method" - [2] "[roxygen-block-3.R:36] argument `duplicate` documented multiple times for R6 method `meth3`" - [3] "[roxygen-block-3.R:36] argument `missing` undocumented for R6 method `meth3()`" - [4] "[roxygen-block-3.R:92] R6 active binding documented multiple times: `duplicate_binding`" - [5] "[roxygen-block-3.R:92] R6 field documented multiple times: `duplicatefield`" - [6] "[roxygen-block-3.R:92] undocumented R6 active binding: `undocumented_binding`" - [7] "[roxygen-block-3.R:92] undocumented R6 fields: `field2`, `undocumented_field`" - [8] "[roxygen-block-3.R:92] undocumented R6 method: `undocumented_method`" - [9] "[roxygen-block-3.R:92] unknown R6 field: `nosuchfield`" + Code + res <- roclet_process(roc, blocks = blocks, env = env, base_path = test_path()) + Condition + Warning: + [roxygen-block-3.R:13] Must use one @param for each argument + x $meth3(duplicate) is documented multiple times + Warning: + [roxygen-block-3.R:13] Must use one @param for each argument + x $meth3(missing) is not documented + Warning: + [roxygen-block-3.R:13] Must use one @return per R6 method + Warning: + [roxygen-block-3.R:92] Undocumented R6 method[s]: undocumented_method + Warning: + [roxygen-block-3.R:92] Undocumented R6 field[s]: field2 and undocumented_field + Warning: + [roxygen-block-3.R:92] R6 field[s] documented multiple times: duplicatefield + Warning: + [roxygen-block-3.R:92] Unknown R6 field[s]: nosuchfield + Warning: + [roxygen-block-3.R:92] Undocumented R6 active binding[s]: undocumented_binding + Warning: + [roxygen-block-3.R:92] R6 active binding[s] documented multiple times: duplicate_binding diff --git a/tests/testthat/test-rd-r6.R b/tests/testthat/test-rd-r6.R index d3240b3ea..2b5121a5c 100644 --- a/tests/testthat/test-rd-r6.R +++ b/tests/testthat/test-rd-r6.R @@ -241,7 +241,7 @@ test_that("R6 edge cases, class without (documented) fields", { block <- parse_text(text)[[1]] rd <- RoxyTopic$new() - expect_warning(topic_add_r6_methods(rd, block, environment())) + expect_snapshot(topic_add_r6_methods(rd, block, environment())) expect_false(grepl("field", format(rd), ignore.case = TRUE)) }) @@ -308,10 +308,7 @@ test_that("warning if no method comes after the docs", { block <- parse_text(text, env = environment())[[1]] rd <- RoxyTopic$new() - expect_warning( - topic_add_r6_methods(rd, block, environment()), - "Cannot find matching R6 method" - ) + expect_snapshot(topic_add_r6_methods(rd, block, environment())) doc <- format(rd) }) @@ -331,19 +328,13 @@ test_that("integration test", { roc <- roclet_preprocess(roclet_find("rd")) - roxy_warnings <- character() - - withCallingHandlers( - res <- roclet_process(roc, blocks = blocks, env = env, base_path = test_path()), - warning = function(w) { - roxy_warnings <<- c(roxy_warnings, w$message) - invokeRestart("muffleWarning") - } + expect_snapshot( + res <- roclet_process(roc, blocks = blocks, env = env, base_path = test_path()) ) - # Warnings - expect_snapshot_output(sort(roxy_warnings)) - + # # Warnings + # expect_snapshot_output(sort(roxy_warnings)) + # tmp <- tempfile() on.exit(unlink(tmp), add = TRUE) for (n in names(res)) { From 67f686272c2212668c065b47cebd877c27685eef Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 09:45:14 -0500 Subject: [PATCH 15/18] Update unknown tag warning --- R/tag.R | 3 ++- tests/testthat/_snaps/tag.md | 4 ++++ tests/testthat/test-tag.R | 10 ++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 tests/testthat/_snaps/tag.md diff --git a/R/tag.R b/R/tag.R index f9ba5578a..35cb681c3 100644 --- a/R/tag.R +++ b/R/tag.R @@ -41,7 +41,8 @@ roxy_tag_parse <- function(x) { #' @export roxy_tag_parse.default <- function(x) { - roxy_tag_warning(x, "unknown tag") + warn_roxy_tag(x, "is not a known tag") + NULL } is.roxy_tag <- function(x) inherits(x, "roxy_tag") diff --git a/tests/testthat/_snaps/tag.md b/tests/testthat/_snaps/tag.md new file mode 100644 index 000000000..cfe8b3757 --- /dev/null +++ b/tests/testthat/_snaps/tag.md @@ -0,0 +1,4 @@ +# warn about unknown tags + + [:2] @unknown is not a known tag + diff --git a/tests/testthat/test-tag.R b/tests/testthat/test-tag.R index 0b8c208b7..184426045 100644 --- a/tests/testthat/test-tag.R +++ b/tests/testthat/test-tag.R @@ -1,3 +1,13 @@ +test_that("warn about unknown tags", { + block <- " + #' @unknown + foo <- function() {} + " + expect_snapshot_warning( + roc_proc_text(rd_roclet(), block) + ) +}) + # Test low-level behaviour ---------------------------------------------------- test_that("braces must balance", { From e3a6b9c596e5bb85906604f12a701f64073a3b0c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Apr 2022 11:20:06 -0500 Subject: [PATCH 16/18] Overall generated tags * Now get roxy_generated_tag() helper that takes a block * All tags must now have file and line * File name resolution happens in rd_section_reexport() so we can use consistent warnings (always mentioning the tag) * Final use of roxy_warning() removed, so inline into export roxy_tag_warning() --- R/block.R | 4 +- R/object-defaults.R | 58 ++++++++++----------- R/object-import.R | 43 ++++++++------- R/object-r6.R | 6 +-- R/rd-find-link-files.R | 21 ++------ R/rd-r6.R | 15 ++++-- R/roxygenize.R | 11 ---- R/tag.R | 33 ++++++++++-- man/roxy_tag.Rd | 5 +- tests/testthat/_snaps/block.md | 6 +-- tests/testthat/_snaps/markdown-link.md | 16 +++--- tests/testthat/_snaps/rd-find-link-files.md | 20 +++++++ tests/testthat/test-markdown-link.R | 2 +- tests/testthat/test-object-import.R | 19 +++---- tests/testthat/test-rd-find-link-files.R | 7 +++ tests/testthat/test-rd-include-rmd.R | 2 +- 16 files changed, 152 insertions(+), 116 deletions(-) create mode 100644 tests/testthat/_snaps/rd-find-link-files.md create mode 100644 tests/testthat/test-rd-find-link-files.R diff --git a/R/block.R b/R/block.R index e71858d41..ad7fdd7f3 100644 --- a/R/block.R +++ b/R/block.R @@ -143,8 +143,8 @@ block_find_object <- function(block, env) { )) # Add in defaults generated from the object - defaults <- object_defaults(object) - defaults <- c(defaults, list(roxy_tag("backref", block$file, block$file))) + defaults <- object_defaults(object, block) + defaults <- c(defaults, list(roxy_generated_tag(block, "backref", block$file))) default_tags <- map_chr(defaults, "tag") defaults <- defaults[!default_tags %in% block_tags(block)] diff --git a/R/object-defaults.R b/R/object-defaults.R index 59736e82a..ae630b6be 100644 --- a/R/object-defaults.R +++ b/R/object-defaults.R @@ -1,14 +1,14 @@ -object_defaults <- function(x) UseMethod("object_defaults") +object_defaults <- function(x, block) UseMethod("object_defaults") #' @export -object_defaults.default <- function(x) list() +object_defaults.default <- function(x, block) list() #' @exportS3Method object_defaults "function" -object_defaults.function <- function(x) { +object_defaults.function <- function(x, block) { list( - roxy_tag("usage", NULL, object_usage(x)), + roxy_generated_tag(block, "usage", object_usage(x)), # Used in process_inherit_params() - roxy_tag(".formals", NULL, names(formals(x$value))) + roxy_generated_tag(block, ".formals", names(formals(x$value))) ) } @@ -25,19 +25,19 @@ object_defaults.s4generic <- object_defaults.function object_defaults.s4method <- object_defaults.function #' @export -object_defaults.data <- function(x) { +object_defaults.data <- function(x, block) { str_out <- rd(object_format(x$value)) list( - roxy_tag("docType", NULL, "data"), - roxy_tag("format", NULL, str_out), - roxy_tag("keywords", NULL, "datasets"), - roxy_tag("usage", NULL, object_usage(x)) + roxy_generated_tag(block, "docType", "data"), + roxy_generated_tag(block, "format", str_out), + roxy_generated_tag(block, "keywords", "datasets"), + roxy_generated_tag(block, "usage", object_usage(x)) ) } #' @export -object_defaults.package <- function(x) { +object_defaults.package <- function(x, block) { desc <- x$value$desc description <- as.character(desc$Description) @@ -48,41 +48,41 @@ object_defaults.package <- function(x) { } list( - roxy_tag("docType", NULL, "package"), - roxy_tag("name", NULL, package_suffix(desc$Package)), + roxy_generated_tag(block, "docType", "package"), + roxy_generated_tag(block, "name", package_suffix(desc$Package)), # "NULL" prevents addition of default aliases, see also #202 - roxy_tag("aliases", NULL, paste("NULL", desc$Package, package_suffix(desc$Package))), - roxy_tag("title", NULL, paste0(desc$Package, ": ", desc$Title)), - roxy_tag("description", NULL, description), - roxy_tag("seealso", NULL, package_seealso(desc)), - roxy_tag("author", NULL, package_authors(desc)) + roxy_generated_tag(block, "aliases", paste("NULL", desc$Package, package_suffix(desc$Package))), + roxy_generated_tag(block, "title", paste0(desc$Package, ": ", desc$Title)), + roxy_generated_tag(block, "description", description), + roxy_generated_tag(block, "seealso", package_seealso(desc)), + roxy_generated_tag(block, "author", package_authors(desc)) ) } #' @export -object_defaults.import <- function(x) { +object_defaults.import <- function(x, block) { list( - roxy_tag("docType", NULL, "import"), - roxy_tag("name", NULL, "reexports"), - roxy_tag("keywords", NULL, "internal"), - roxy_tag("title", NULL, "Objects exported from other packages"), - roxy_tag(".reexport", NULL, list(pkg = x$value$pkg, fun = x$value$fun)) + roxy_generated_tag(block, "docType", "import"), + roxy_generated_tag(block, "name", "reexports"), + roxy_generated_tag(block, "keywords", "internal"), + roxy_generated_tag(block, "title", "Objects exported from other packages"), + roxy_generated_tag(block, ".reexport", list(pkg = x$value$pkg, fun = x$value$fun)) ) } #' @export -object_defaults.s4class <- function(x) { +object_defaults.s4class <- function(x, block) { list( - roxy_tag("docType", NULL, "class") + roxy_generated_tag(block, "docType", "class") ) } #' @export -object_defaults.rcclass <- function(x) { +object_defaults.rcclass <- function(x, block) { list( - roxy_tag("docType", NULL, "class"), + roxy_generated_tag(block, "docType", "class"), if (!is.null(x$methods)) - roxy_tag(".methods", NULL, x$methods) + roxy_generated_tag(block, ".methods", x$methods) ) } diff --git a/R/object-import.R b/R/object-import.R index 8e50e634e..eb6531873 100644 --- a/R/object-import.R +++ b/R/object-import.R @@ -1,37 +1,44 @@ # Re-export ---------------------------------------------------------------- -rd_section_reexport <- function(pkg, fun) { - stopifnot(is.character(pkg), is.character(fun)) +rd_section_reexport <- function(pkg, fun, file) { + stopifnot(is.character(pkg), is.character(fun), is.character(file)) stopifnot(length(pkg) == length(fun)) - rd_section("reexport", list(pkg = pkg, fun = fun)) + rd_section("reexport", list(pkg = pkg, fun = fun, file = file)) } #' @export roxy_tag_rd.roxy_tag_.reexport <- function(x, base_path, env) { - rd_section_reexport(x$val$pkg, x$val$fun) + file <- find_topic_filename(x$val$pkg, x$val$fun, tag = x) + rd_section_reexport(x$val$pkg, x$val$fun, file) } #' @export merge.rd_section_reexport <- function(x, y, ...) { stopifnot(identical(class(x), class(y))) - rd_section_reexport(c(x$value$pkg, y$value$pkg), c(x$value$fun, y$value$fun)) + rd_section_reexport( + c(x$value$pkg, y$value$pkg), + c(x$value$fun, y$value$fun), + c(x$value$file, y$value$file) + ) } #' @export format.rd_section_reexport <- function(x, ...) { - pkgs <- split(x$value$fun, x$value$pkg) - pkg_links <- map2(names(pkgs), pkgs, function(pkg, funs) { - funs <- sort_c(funs) - files <- map_chr( - funs, - try_find_topic_in_package, - pkg = pkg, - where = " in re-export" - ) + + info <- data.frame( + pkg = x$value$pkg, + fun = x$value$fun, + file = x$value$file, + stringsAsFactors = FALSE + ) + + pkgs <- split(info, x$value$pkg) + pkg_links <- map(pkgs, function(pkg) { + pkg <- pkg[order(pkg$fun), ] links <- paste0( - "\\code{\\link[", pkg, - ifelse(files == funs, "", paste0(":", files)), - "]{", escape(funs), "}}", + "\\code{\\link[", pkg$pkg, + ifelse(pkg$file == pkg$fun, "", paste0(":", pkg$file)), + "]{", escape(pkg$fun), "}}", collapse = ", ") - paste0("\\item{", pkg, "}{", links, "}") + paste0("\\item{", pkg$pkg, "}{", links, "}") }) paste0( diff --git a/R/object-r6.R b/R/object-r6.R index 51c864d95..38667440e 100644 --- a/R/object-r6.R +++ b/R/object-r6.R @@ -1,10 +1,10 @@ #' @export -object_defaults.r6class <- function(x) { +object_defaults.r6class <- function(x, block) { r6on <- roxy_meta_get("r6", TRUE) if (isTRUE(r6on)) { list( - roxy_tag("docType", NULL, NULL), - roxy_tag(".r6data", NULL, extract_r6_data(x$value)) + roxy_generated_tag(block, "docType", NULL), + roxy_generated_tag(block, ".r6data", extract_r6_data(x$value)) ) } else { NextMethod() diff --git a/R/rd-find-link-files.R b/R/rd-find-link-files.R index c83eaa4a2..031f67449 100644 --- a/R/rd-find-link-files.R +++ b/R/rd-find-link-files.R @@ -24,7 +24,7 @@ find_topic_filename <- function(pkg, topic, tag = NULL) { if (is.na(pkg) || identical(roxy_meta_get("current_package"), pkg)) { topic } else { - try_find_topic_in_package(pkg, topic, tag = tag) + try_find_topic_in_package(pkg, topic, tag) } } @@ -51,30 +51,17 @@ find_topic_in_package <- function(pkg, topic) { basename(utils::help((raw_topic), (pkg))[1]) } -try_find_topic_in_package <- function(pkg, topic, where = "", tag = NULL) { +try_find_topic_in_package <- function(pkg, topic, tag) { path <- tryCatch( find_topic_in_package(pkg, topic), error = function(err) { - msg <- paste0( - "Link to unavailable package", where, ": ", pkg, "::", topic, ".\n", - err$message - ) - if (is.null(tag)) { - roxy_warning(msg) - } else { - roxy_tag_warning(tag, msg) - } + warn_roxy_tag(tag, "refers to unavailable topic {pkg}::{topic}", parent = err) topic } ) if (is.na(path)) { - msg <- paste0("Link to unknown topic", where, ": ", pkg, "::", topic) - if (is.null(tag)) { - roxy_warning(msg) - } else { - roxy_tag_warning(tag, msg) - } + warn_roxy_tag(tag, "refers to unavailable topic {pkg}::{topic}") topic } else { path diff --git a/R/rd-r6.R b/R/rd-r6.R index 6e74c0d06..3f8eb86d7 100644 --- a/R/rd-r6.R +++ b/R/rd-r6.R @@ -26,7 +26,7 @@ topic_add_r6_methods <- function(rd, block, env) { del <- c(del, i) } - methods <- add_default_methods(methods) + methods <- add_default_methods(methods, block) nodoc <- map_int(methods$tags, length) == 0 if (any(nodoc)) { @@ -61,14 +61,19 @@ topic_add_r6_methods <- function(rd, block, env) { } } -add_default_methods <- function(methods) { +add_default_methods <- function(methods, block) { defaults <- list( clone = list( - roxy_tag_parse(roxy_tag( + roxy_generated_tag( + block, "description", "The objects of this class are cloneable with this method." - )), - roxy_tag_parse(roxy_tag("param", "deep Whether to make a deep clone.")) + ), + roxy_generated_tag( + block, + "param", + list(name = "deep", description = "Whether to make a deep clone.") + ) ) ) diff --git a/R/roxygenize.R b/R/roxygenize.R index 76bb1a762..cf811bc12 100644 --- a/R/roxygenize.R +++ b/R/roxygenize.R @@ -108,14 +108,3 @@ roxygen_setup <- function(base_path) { is_first } - -roxy_warning <- function(..., file = NA, line = NA) { - message <- paste0( - if (!is.na(file)) paste0("[", file, ":", line, "] "), - ..., - collapse = " " - ) - - warning(message, call. = FALSE, immediate. = TRUE) - NULL -} diff --git a/R/tag.R b/R/tag.R index 35cb681c3..583c7f102 100644 --- a/R/tag.R +++ b/R/tag.R @@ -1,7 +1,8 @@ #' `roxy_tag` S3 constructor #' #' `roxy_tag()` is the constructor for tag objects. -#' `roxy_tag_warning()` generates a warning that gives the location of the tag. +#' `roxy_tag_warning()` is superseded by `warn_roxy_tag()`; use to generate a +#' warning that includes the location of the tag. #' #' @section Methods: #' Define a method for `roxy_tag_parse` to support new tags. See [tag_parsers] @@ -15,7 +16,7 @@ #' @param val Parsed tag value, typically a character vector, but sometimes #' a list. Usually filled in by `tag_parsers` #' @param file,line Location of the tag -roxy_tag <- function(tag, raw, val = NULL, file = NA_character_, line = NA_integer_) { +roxy_tag <- function(tag, raw, val = NULL, file = NA_character_, line = NA_character_) { structure( list( file = file, @@ -28,6 +29,16 @@ roxy_tag <- function(tag, raw, val = NULL, file = NA_character_, line = NA_integ ) } +roxy_generated_tag <- function(block, tag, val) { + roxy_tag( + tag = tag, + raw = NULL, + val = val, + file = block$file, + line = block$line + ) +} + roxy_test_tag <- function(raw = "", val = NULL) { roxy_tag("test", raw = raw, val = val, file = "test.R", line = 1) } @@ -94,12 +105,26 @@ print.roxy_tag <- function(x, ...) { #' @export #' @rdname roxy_tag roxy_tag_warning <- function(x, ...) { - roxy_warning(file = x$file, line = x$line, "@", x$tag, " ", ...) + # Should no longer be used internally + + message <- paste0( + if (!is.na(x$file)) paste0("[", x$file, ":", x$line, "] "), + ..., + collapse = " " + ) + + warning(message, call. = FALSE, immediate. = TRUE) + NULL } #' @export #' @rdname roxy_tag warn_roxy_tag <- function(tag, message, ...) { - message[[1]] <- paste0("[", tag$file, ":", tag$line, "] @", tag$tag, " ", message[[1]]) + message[[1]] <- paste0( + "[", tag$file, ":", tag$line, "] ", + "@", tag$tag, " ", + if (is.null(tag$raw)) ("(automatically generated) "), + message[[1]] + ) cli::cli_warn(message, ..., .envir = parent.frame()) } diff --git a/man/roxy_tag.Rd b/man/roxy_tag.Rd index 79c8ff17e..abcd615e7 100644 --- a/man/roxy_tag.Rd +++ b/man/roxy_tag.Rd @@ -7,7 +7,7 @@ \alias{warn_roxy_tag} \title{\code{roxy_tag} S3 constructor} \usage{ -roxy_tag(tag, raw, val = NULL, file = NA_character_, line = NA_integer_) +roxy_tag(tag, raw, val = NULL, file = NA_character_, line = NA_character_) roxy_tag_parse(x) @@ -30,7 +30,8 @@ a list. Usually filled in by \code{tag_parsers}} } \description{ \code{roxy_tag()} is the constructor for tag objects. -\code{roxy_tag_warning()} generates a warning that gives the location of the tag. +\code{roxy_tag_warning()} is superseded by \code{warn_roxy_tag()}; use to generate a +warning that includes the location of the tag. } \section{Methods}{ diff --git a/tests/testthat/_snaps/block.md b/tests/testthat/_snaps/block.md index a5c13c17e..51f54c8a3 100644 --- a/tests/testthat/_snaps/block.md +++ b/tests/testthat/_snaps/block.md @@ -5,9 +5,9 @@ [line: 1] @title 'This is a title' {parsed} [line: 3] @param 'x,y A number' {parsed} [line: 4] @export '' {parsed} - [????:???] @usage '' {parsed} - [????:???] @.formals '' {parsed} - [????:???] @backref '' {parsed} + [line: 5] @usage '' {parsed} + [line: 5] @.formals '' {parsed} + [line: 5] @backref '' {parsed} $call f <- function(x, y) x + y $object $topic f diff --git a/tests/testthat/_snaps/markdown-link.md b/tests/testthat/_snaps/markdown-link.md index 1561fdb64..62f1a32c2 100644 --- a/tests/testthat/_snaps/markdown-link.md +++ b/tests/testthat/_snaps/markdown-link.md @@ -4,7 +4,7 @@ markdown("[`foo` bar][x]", tag = tag) Condition Warning: - [foo.R:10] @title markdown: links must contain plain text + [foo.R:10] @title (automatically generated) markdown: links must contain plain text Problematic link: x Output [1] "" @@ -12,7 +12,7 @@ markdown("[__baz__][x]", tag = tag) Condition Warning: - [foo.R:10] @title markdown: links must contain plain text + [foo.R:10] @title (automatically generated) markdown: links must contain plain text Problematic link: x Output [1] "" @@ -25,13 +25,15 @@ 1]] Condition Warning: - [:4] @description Link to unavailable package: 11pkg::function. - there is no package called '11pkg' + [:4] @description refers to unavailable topic 11pkg::function + Caused by error in `find.package()`: + ! there is no package called '11pkg' Warning: - [:4] @description Link to unavailable package: 11pkg::object. - there is no package called '11pkg' + [:4] @description refers to unavailable topic 11pkg::object + Caused by error in `find.package()`: + ! there is no package called '11pkg' --- - [:4] @description Link to unknown topic: stringr::bar111 + [:4] @description refers to unavailable topic stringr::bar111 diff --git a/tests/testthat/_snaps/rd-find-link-files.md b/tests/testthat/_snaps/rd-find-link-files.md new file mode 100644 index 000000000..93170608c --- /dev/null +++ b/tests/testthat/_snaps/rd-find-link-files.md @@ -0,0 +1,20 @@ +# generates informative warnings + + Code + tag <- roxy_test_tag() + find_topic_filename("11papaya", "foo", tag) + Condition + Warning: + [test.R:1] @test refers to unavailable topic 11papaya::foo + Caused by error in `find.package()`: + ! there is no package called '11papaya' + Output + [1] "foo" + Code + find_topic_filename("stringr", "foofofofoo", tag) + Condition + Warning: + [test.R:1] @test refers to unavailable topic stringr::foofofofoo + Output + [1] "foofofofoo" + diff --git a/tests/testthat/test-markdown-link.R b/tests/testthat/test-markdown-link.R index f6297a5b2..c5976fabd 100644 --- a/tests/testthat/test-markdown-link.R +++ b/tests/testthat/test-markdown-link.R @@ -63,7 +63,7 @@ test_that("{ and } in links are escaped (#1259)", { }) test_that("non-text nodes in links fails", { - tag <- roxy_tag("title", NULL, file = "foo.R", line = 10) + tag <- roxy_tag("title", NULL, NULL, file = "foo.R", line = 10) expect_snapshot({ markdown("[`foo` bar][x]", tag = tag) diff --git a/tests/testthat/test-object-import.R b/tests/testthat/test-object-import.R index a9f266995..c1c253b55 100644 --- a/tests/testthat/test-object-import.R +++ b/tests/testthat/test-object-import.R @@ -5,7 +5,7 @@ test_that("exporting a call to :: produces re-exports documentation", { expect_equal( out$get_section("reexport"), - rd_section_reexport("testthat", "auto_test") + rd_section_reexport("testthat", "auto_test", "auto_test") ) expect_equal(out$get_value("title"), "Objects exported from other packages") expect_equal(out$get_value("keyword"), "internal") @@ -24,7 +24,11 @@ test_that("multiple re-exports are combined", { expect_equal( out$get_section("reexport"), - rd_section_reexport(c("testthat", "testthat"), c("expect_lt", "expect_gt")) + rd_section_reexport( + c("testthat", "testthat"), + c("expect_lt", "expect_gt"), + c("comparison-expectations", "comparison-expectations") + ) ) }) @@ -38,14 +42,3 @@ test_that("description generated correctly", { expect_null(out$get_section("description")) }) - -test_that("warnings for unknown packages and objects", { - expect_warning( - format(rd_section_reexport("11papaya", "fun")), - "[uU]navailable package in re-export" - ) - expect_warning( - format(rd_section_reexport("stringr", "12345543221")), - "[uU]nknown topic in re-export" - ) -}) diff --git a/tests/testthat/test-rd-find-link-files.R b/tests/testthat/test-rd-find-link-files.R new file mode 100644 index 000000000..c84ee915d --- /dev/null +++ b/tests/testthat/test-rd-find-link-files.R @@ -0,0 +1,7 @@ +test_that("generates informative warnings", { + expect_snapshot({ + tag <- roxy_test_tag() + find_topic_filename("11papaya", "foo", tag) + find_topic_filename("stringr", "foofofofoo", tag) + }) +}) diff --git a/tests/testthat/test-rd-include-rmd.R b/tests/testthat/test-rd-include-rmd.R index d026b6b3e..ee35c0662 100644 --- a/tests/testthat/test-rd-include-rmd.R +++ b/tests/testthat/test-rd-include-rmd.R @@ -140,7 +140,7 @@ test_that("links to functions, with anchors", { test_that("empty Rmd", { tmp <- tempfile() on.exit(unlink(tmp), add = TRUE) - tag <- roxy_tag("includeRmd", tmp) + tag <- roxy_test_tag() cat("", sep = "", file = tmp) expect_equal(rmd_eval_rd(tmp, tag), structure("", names = "")) From 837e41ba9af622078452e4e4a93859c653b42bdb Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 7 Apr 2022 07:33:45 -0500 Subject: [PATCH 17/18] Feedback from code review --- R/markdown-link.R | 4 ++-- R/markdown-state.R | 2 +- R/markdown.R | 6 ++++-- R/rd-r6.R | 12 ++++++------ R/rd-raw.R | 2 +- tests/testthat/_snaps/markdown-link.md | 8 ++++---- tests/testthat/_snaps/rd-r6.md | 14 +++++++------- tests/testthat/test-rd-r6.R | 3 --- 8 files changed, 25 insertions(+), 26 deletions(-) diff --git a/R/markdown-link.R b/R/markdown-link.R index 04f66f00b..e21e08b80 100644 --- a/R/markdown-link.R +++ b/R/markdown-link.R @@ -106,8 +106,8 @@ parse_link <- function(destination, contents, state) { incorrect <- setdiff(unique(xml_name(contents)), c("text", "softbreak", "linebreak")) warn_roxy_tag(state$tag, c( - "markdown: links must contain plain text", - "Problematic link: {destination}" + "markdown links must contain plain text", + i = "Problematic link: {destination}" )) return("") } diff --git a/R/markdown-state.R b/R/markdown-state.R index 88e2dff9f..e02f748ad 100644 --- a/R/markdown-state.R +++ b/R/markdown-state.R @@ -19,7 +19,7 @@ markdown_on <- function(value = NULL) { local_markdown <- function(env = parent.frame()) { old <- markdown_env$`markdown-support` markdown_on(TRUE) - withr::defer(markdown_on(FALSE), envir = env) + withr::defer(markdown_on(old), envir = env) } markdown_activate <- function(tags) { diff --git a/R/markdown.R b/R/markdown.R index 80e6af67a..9b63b4066 100644 --- a/R/markdown.R +++ b/R/markdown.R @@ -185,7 +185,8 @@ mdxml_node_to_rd <- function(xml, state) { ! xml_type(xml) %in% c("text", "element")) { warn_roxy_tag(state$tag, c( "markdown translation failed", - "internal error" + x = "Unexpected internal error", + i = "Please file an issue at https://github.com/r-lib/roxygen2/issues" )) return("") } @@ -226,7 +227,8 @@ mdxml_node_to_rd <- function(xml, state) { mdxml_unknown <- function(xml, tag) { warn_roxy_tag(tag, c( "markdown translation failed", - x = "Internal error: unknown xml node {xml_name(xml)}" + x = "Internal error: unknown xml node {xml_name(xml)}", + i = "Please file an issue at https://github.com/r-lib/roxygen2/issues" )) escape_comment(xml_text(xml)) } diff --git a/R/rd-r6.R b/R/rd-r6.R index 3f8eb86d7..a953872ed 100644 --- a/R/rd-r6.R +++ b/R/rd-r6.R @@ -30,7 +30,7 @@ topic_add_r6_methods <- function(rd, block, env) { nodoc <- map_int(methods$tags, length) == 0 if (any(nodoc)) { - warn_roxy_block(block, "Undocumented R6 method[s]: {methods$name[nodoc]}") + warn_roxy_block(block, "Undocumented R6 method{?s}: {methods$name[nodoc]}") } block$tags[del] <- NULL @@ -124,19 +124,19 @@ r6_fields <- function(block, r6data) { # Check for missing fields miss <- setdiff(fields, docd) if (length(miss) > 0) { - warn_roxy_block(block, "Undocumented R6 field[s]: {miss}") + warn_roxy_block(block, "Undocumented R6 field{?s}: {miss}") } # Check for duplicate fields dup <- unique(docd[duplicated(docd)]) if (length(dup) > 0) { - warn_roxy_block(block, "R6 field[s] documented multiple times: {dup}") + warn_roxy_block(block, "R6 field{?s} documented multiple times: {dup}") } # Check for extra fields xtra <- setdiff(docd, fields) if (length(xtra) > 0) { - warn_roxy_block(block, "Unknown R6 field[s]: {xtra}") + warn_roxy_block(block, "Unknown R6 field{?s}: {xtra}") } if (length(docd) == 0) return() @@ -170,13 +170,13 @@ r6_active_bindings <- function(block, r6data) { # Check for missing bindings miss <- setdiff(active, docd) if (length(miss) > 0) { - warn_roxy_block(block, "Undocumented R6 active binding[s]: {miss}") + warn_roxy_block(block, "Undocumented R6 active binding{?s}: {miss}") } # Check for duplicate bindings dup <- unique(docd[duplicated(docd)]) if (length(dup) > 0) { - warn_roxy_block(block, "R6 active binding[s] documented multiple times: {dup}") + warn_roxy_block(block, "R6 active binding{?s} documented multiple times: {dup}") } if (length(docd) == 0) return() diff --git a/R/rd-raw.R b/R/rd-raw.R index 59691c444..6b6f28190 100644 --- a/R/rd-raw.R +++ b/R/rd-raw.R @@ -16,7 +16,7 @@ format.rd_section_rawRd <- function(x, ...) { paste(x$value, collapse = "\n") } -roxy_tag_eval <- function(tag, env = new.env(emptyenv())) { +roxy_tag_eval <- function(tag, env = new.env(parent = baseenv())) { tryCatch({ out <- eval(tag$val, envir = env) diff --git a/tests/testthat/_snaps/markdown-link.md b/tests/testthat/_snaps/markdown-link.md index 62f1a32c2..6ac0737ed 100644 --- a/tests/testthat/_snaps/markdown-link.md +++ b/tests/testthat/_snaps/markdown-link.md @@ -4,16 +4,16 @@ markdown("[`foo` bar][x]", tag = tag) Condition Warning: - [foo.R:10] @title (automatically generated) markdown: links must contain plain text - Problematic link: x + [foo.R:10] @title (automatically generated) markdown links must contain plain text + i Problematic link: x Output [1] "" Code markdown("[__baz__][x]", tag = tag) Condition Warning: - [foo.R:10] @title (automatically generated) markdown: links must contain plain text - Problematic link: x + [foo.R:10] @title (automatically generated) markdown links must contain plain text + i Problematic link: x Output [1] "" diff --git a/tests/testthat/_snaps/rd-r6.md b/tests/testthat/_snaps/rd-r6.md index a40d71a16..7597e9da2 100644 --- a/tests/testthat/_snaps/rd-r6.md +++ b/tests/testthat/_snaps/rd-r6.md @@ -4,7 +4,7 @@ topic_add_r6_methods(rd, block, environment()) Condition Warning: - [:7] Undocumented R6 field[s]: undocumented_field + [:7] Undocumented R6 field: undocumented_field # warning if no method comes after the docs @@ -28,15 +28,15 @@ Warning: [roxygen-block-3.R:13] Must use one @return per R6 method Warning: - [roxygen-block-3.R:92] Undocumented R6 method[s]: undocumented_method + [roxygen-block-3.R:92] Undocumented R6 method: undocumented_method Warning: - [roxygen-block-3.R:92] Undocumented R6 field[s]: field2 and undocumented_field + [roxygen-block-3.R:92] Undocumented R6 fields: field2 and undocumented_field Warning: - [roxygen-block-3.R:92] R6 field[s] documented multiple times: duplicatefield + [roxygen-block-3.R:92] R6 field documented multiple times: duplicatefield Warning: - [roxygen-block-3.R:92] Unknown R6 field[s]: nosuchfield + [roxygen-block-3.R:92] Unknown R6 field: nosuchfield Warning: - [roxygen-block-3.R:92] Undocumented R6 active binding[s]: undocumented_binding + [roxygen-block-3.R:92] Undocumented R6 active binding: undocumented_binding Warning: - [roxygen-block-3.R:92] R6 active binding[s] documented multiple times: duplicate_binding + [roxygen-block-3.R:92] R6 active binding documented multiple times: duplicate_binding diff --git a/tests/testthat/test-rd-r6.R b/tests/testthat/test-rd-r6.R index 2b5121a5c..5e14667ea 100644 --- a/tests/testthat/test-rd-r6.R +++ b/tests/testthat/test-rd-r6.R @@ -332,9 +332,6 @@ test_that("integration test", { res <- roclet_process(roc, blocks = blocks, env = env, base_path = test_path()) ) - # # Warnings - # expect_snapshot_output(sort(roxy_warnings)) - # tmp <- tempfile() on.exit(unlink(tmp), add = TRUE) for (n in names(res)) { From 7f598e913a0edc16b380d13c9e48c7fbcc98bd98 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 7 Apr 2022 07:44:13 -0500 Subject: [PATCH 18/18] Add news bullet --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 5e87e9868..b2272f289 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # roxygen2 (development version) +* All warning messages have been reviewed to hopefully be more informative + and actionable (#1317). + * DOIs in the `URL` field of the `DESCRIPTION` are now converted to Rd's special `\doi{}` tag (@ThierryO, #1296).