From a3e3908a9404e7c0b32e6a8be5fcb9cec5cbe8a7 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 19 Jan 2024 09:08:47 -0800 Subject: [PATCH] Restore old behavior of @importFrom in some edge cases (#1574) Fixes #1570 Co-authored-by: Hadley Wickham --- NEWS.md | 6 ++++ R/namespace.R | 8 ++++-- R/utils.R | 1 + tests/testthat/_snaps/namespace.md | 11 ++------ tests/testthat/test-namespace.R | 45 +++++++++++++++++++++++++----- 5 files changed, 54 insertions(+), 17 deletions(-) diff --git a/NEWS.md b/NEWS.md index fb8da8949..9bef15bb6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,12 @@ * `@family` lists are now ordered more carefully, "foo1" comes after "foo" (#1563, @krlmlr). +* `@importFrom` works again for quoted non-syntactic names, e.g. + `@importFrom magrittr "%>%"` or ``@importFrom rlang `:=` ``, after being broken + in 7.3.0 (#1570, @MichaelChirico). The unquoted form `@importFrom magrittr %>%` + continues to work. Relatedly, `@importFrom` directives not matching any known + functions (e.g. `@importFrom utils plot pdf`) produce valid NAMESPACE files again. + # roxygen2 7.3.0 ## New features diff --git a/R/namespace.R b/R/namespace.R index d33e56b05..3201fe1d9 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -268,9 +268,13 @@ roxy_tag_ns.roxy_tag_importFrom <- function(x, block, env) { pkg <- x$val[1L] if (requireNamespace(pkg, quietly = TRUE)) { importing <- x$val[-1L] - unknown_idx <- !importing %in% getNamespaceExports(pkg) + # be sure to match '%>%', `%>%`, "%>%" all to %>% given by getNamespaceExports, #1570 + unknown_idx <- !strip_quotes(importing) %in% getNamespaceExports(pkg) if (any(unknown_idx)) { - warn_roxy_tag(x, "Excluding unknown {cli::qty(sum(unknown_idx))} export{?s} in from {.package {pkg}}: {.code {importing[unknown_idx]}}") + warn_roxy_tag(x, "Excluding unknown {cli::qty(sum(unknown_idx))} export{?s} from {.package {pkg}}: {.code {importing[unknown_idx]}}") + if (all(unknown_idx)) { + return(NULL) + } x$val <- c(pkg, importing[!unknown_idx]) } } diff --git a/R/utils.R b/R/utils.R index 51a0ce211..b1ccc91c0 100644 --- a/R/utils.R +++ b/R/utils.R @@ -179,3 +179,4 @@ auto_quote <- function(x) { is_syntactic <- function(x) make.names(x) == x has_quotes <- function(x) str_detect(x, "^(`|'|\").*\\1$") +strip_quotes <- function(x) str_replace(x, "^(`|'|\")(.*)\\1$", "\\2") diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md index 8a069bad9..a9f47daf9 100644 --- a/tests/testthat/_snaps/namespace.md +++ b/tests/testthat/_snaps/namespace.md @@ -78,24 +78,19 @@ Message x :2: @evalNamespace must evaluate to a character vector. -# Invalid imports throw a helpful error +# invalid imports generate helpful message Code out <- roc_proc_text(namespace_roclet(), block) Message - x :2: @importFrom Excluding unknown export in from utils: `InvalidUtilsFunction`. + x :2: @importFrom Excluding unknown export from utils: `InvalidUtilsFunction1`. --- Code out <- roc_proc_text(namespace_roclet(), block) Message - x :2: @importFrom Excluding unknown exports in from utils: `InvalidUtilsFunction1` and `InvalidUtilsFunction2`. - ---- - - Code - out <- roc_proc_text(namespace_roclet(), block) + x :2: @importFrom Excluding unknown exports from utils: `InvalidUtilsFunction1` and `InvalidUtilsFunction2`. # warns if S3 method not documented diff --git a/tests/testthat/test-namespace.R b/tests/testthat/test-namespace.R index c6322cdec..029e13630 100644 --- a/tests/testthat/test-namespace.R +++ b/tests/testthat/test-namespace.R @@ -394,32 +394,63 @@ test_that("can extract non-imports from namespace preserving source", { expect_equal(namespace_exports(path), lines[c(1:3, 5)]) }) -test_that("Invalid imports throw a helpful error", { +test_that("invalid imports generate correct declarations", { + # No matched functions --> no output + block <- " + #' @importFrom utils InvalidUtilsFunction + NULL + " + expect_message(out <- roc_proc_text(namespace_roclet(), block)) + expect_equal(out, character()) + + # Matched functions --> only drop unmatched functions block <- " #' @importFrom utils head InvalidUtilsFunction NULL " - expect_snapshot(out <- roc_proc_text(namespace_roclet(), block)) + expect_message(out <- roc_proc_text(namespace_roclet(), block)) expect_equal(out, "importFrom(utils,head)") +}) + +test_that("invalid imports generate helpful message", { + block <- " + #' @importFrom utils head InvalidUtilsFunction1 + NULL + " + expect_snapshot(out <- roc_proc_text(namespace_roclet(), block)) - # pluralization block <- " #' @importFrom utils head InvalidUtilsFunction1 InvalidUtilsFunction2 NULL " expect_snapshot(out <- roc_proc_text(namespace_roclet(), block)) - expect_equal(out, "importFrom(utils,head)") +}) - # If the package is not available at roxygenize() run time, nothing we can do +test_that("nothing we can do if package isn't installed", { block <- " #' @importFrom AnUnknownUnavailablePackage Unchecked NULL " - expect_snapshot(out <- roc_proc_text(namespace_roclet(), block)) + expect_no_message(out <- roc_proc_text(namespace_roclet(), block)) expect_equal(out, "importFrom(AnUnknownUnavailablePackage,Unchecked)") - }) +test_that("non-syntactic imports can use multiple quoting forms", { + lines <- c( + "#' @importFrom stringr %>%", + "#' @importFrom stringr `%>%`", + "#' @importFrom stringr '%>%'", + "#' @importFrom stringr \"%>%\"", + "NULL" + ) + + import <- expect_no_warning(roc_proc_text(namespace_roclet(), lines)) + expect_equal(import, c( + "importFrom(stringr,\"%>%\")", + "importFrom(stringr,'%>%')", + "importFrom(stringr,`%>%`)" + )) +}) # warn_missing_s3_exports -------------------------------------------------