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 all 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: 3 additions & 8 deletions tests/testthat/_snaps/namespace.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,24 +78,19 @@
Message
x <text>: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 <text>:2: @importFrom Excluding unknown export in from utils: `InvalidUtilsFunction`.
x <text>:2: @importFrom Excluding unknown export from utils: `InvalidUtilsFunction1`.

---

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

---

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

# warns if S3 method not documented

Expand Down
45 changes: 38 additions & 7 deletions tests/testthat/test-namespace.R
Original file line number Diff line number Diff line change
Expand Up @@ -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", {
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
# 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,\"%>%\")",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for my edification, any reason to prefer this to

Suggested change
"importFrom(stringr,\"%>%\")",
'importFrom(stringr,"%>%")',

I like this for keeping the 3 lines of code aligned vertically / no need to visually parse escapes.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have particularly strong feelings here but I like that currently all the strings use ", and are consistent with the values in lines. I saw you used ' for the input "%>%", but I didn't like having to escape #'.

"importFrom(stringr,'%>%')",
"importFrom(stringr,`%>%`)"
))
})

# warn_missing_s3_exports -------------------------------------------------

Expand Down
Loading