Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore old behavior of @importFrom in some edge cases #1574

Merged
merged 8 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions R/namespace.R
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
}
Copy link
Member

Choose a reason for hiding this comment

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

One more thought: how do the different types of quoting affect the NAMESPACE? Are they normalised elsewhere, or is it ok to use any of the forms without quoting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean from roxygen2 perspective or base R?

Answering for the latter: NAMESPACE needs to parse as R, so importFrom(magrittr, %>%) won't work.

So for roxygen2, my understanding is @importFrom magrittr %>% will add quotes for convenience (that's the helper auto_quotes), but quoted forms are dropped in as-is.

Expand Down
1 change: 1 addition & 0 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -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")
11 changes: 9 additions & 2 deletions tests/testthat/_snaps/namespace.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,21 @@
Code
out <- roc_proc_text(namespace_roclet(), block)
Message
x <text>:2: @importFrom Excluding unknown export in from utils: `InvalidUtilsFunction`.
x <text>:2: @importFrom Excluding unknown export from utils: `InvalidUtilsFunction`.

---

Code
out <- roc_proc_text(namespace_roclet(), block)
Message
x <text>:2: @importFrom Excluding unknown exports in from utils: `InvalidUtilsFunction1` and `InvalidUtilsFunction2`.
x <text>:2: @importFrom Excluding unknown export from utils: `InvalidUtilsFunction`.

---

Code
out <- roc_proc_text(namespace_roclet(), block)
Message
x <text>:2: @importFrom Excluding unknown exports from utils: `InvalidUtilsFunction1` and `InvalidUtilsFunction2`.

---

Expand Down
26 changes: 26 additions & 0 deletions tests/testthat/test-namespace.R
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,15 @@ test_that("can extract non-imports from namespace preserving source", {
})

test_that("Invalid imports throw a helpful error", {
# No matched functions --> no output
block <- "
#' @importFrom utils InvalidUtilsFunction
NULL
"
expect_snapshot(out <- roc_proc_text(namespace_roclet(), block))
expect_equal(out, character())

# Matched functions --> only drop unmatched functions
block <- "
#' @importFrom utils head InvalidUtilsFunction
NULL
Expand All @@ -418,6 +427,23 @@ test_that("Invalid imports throw a helpful error", {
expect_snapshot(out <- roc_proc_text(namespace_roclet(), block))
expect_equal(out, "importFrom(AnUnknownUnavailablePackage,Unchecked)")

# make sure to match several forms of non-syntactic names
expect_no_warning(expect_equal(
roc_proc_text(namespace_roclet(), "#' @importFrom stringr %>%\nNULL"),
'importFrom(stringr,"%>%")'
))
hadley marked this conversation as resolved.
Show resolved Hide resolved
expect_no_warning(expect_equal(
roc_proc_text(namespace_roclet(), "#' @importFrom stringr '%>%'\nNULL"),
"importFrom(stringr,'%>%')"
))
expect_no_warning(expect_equal(
roc_proc_text(namespace_roclet(), "#' @importFrom stringr `%>%`\nNULL"),
"importFrom(stringr,`%>%`)"
))
expect_no_warning(expect_equal(
roc_proc_text(namespace_roclet(), '#\' @importFrom stringr "%>%"\nNULL'),
'importFrom(stringr,"%>%")'
))
})


Expand Down
Loading