Skip to content

Commit

Permalink
Correctly interpolate dangerous string into cli warning (#1576)
Browse files Browse the repository at this point in the history
Fixes #1575
  • Loading branch information
hadley committed Jan 22, 2024
1 parent f52889c commit 036e74a
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 15 deletions.
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,4 @@ Config/testthat/parallel: TRUE
Encoding: UTF-8
Language: en-GB
Roxygen: list(markdown = TRUE, load = "installed")
RoxygenNote: 7.2.3.9000
RoxygenNote: 7.3.0.9000
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# roxygen2 (development version)

* S3 method export warning no longer fails if class contains `{` or `}` (#1575).

* `@family` lists are now ordered more carefully, "foo1" comes after "foo" (#1563, @krlmlr).

* Re-runs of `namespace_roclet()` fixed for packages using multi-line `@rawNamespace` directives (#1572, @MichaelChirico).

* `@importFrom` works again for quoted non-syntactic names, e.g.
Expand Down
10 changes: 7 additions & 3 deletions R/namespace.R
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,11 @@ warn_missing_s3_exports <- function(blocks, env) {

undocumented <- methods[!methods %in% s3objects]
srcrefs <- map(undocumented, attr, "srcref")
messages <- paste0("S3 method `", names(undocumented) , "` needs @export or @exportS3method tag")
map2(undocumented, messages, warn_roxy_function)
}

map2(undocumented, names(undocumented), function(fun, name) {
warn_roxy_function(
fun,
"S3 method {.arg {name}} needs @export or @exportS3method tag"
)
})
}
12 changes: 8 additions & 4 deletions tests/testthat/_snaps/namespace.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,15 @@
# warns if S3 method not documented

Code
roc_proc_text(namespace_roclet(),
"\n foo <- function(x) UseMethod('foo')\n foo.numeric <- function(x) 1\n\n mean.myclass <- function(x) 2\n ")
. <- roc_proc_text(namespace_roclet(), block)
Message
x <text>:5: S3 method `mean.myclass` needs @export or @exportS3method tag.
x <text>:3: S3 method `foo.numeric` needs @export or @exportS3method tag.
Output
character(0)

---

Code
. <- roc_proc_text(namespace_roclet(), block)
Message
x <text>:3: S3 method `foo.{` needs @export or @exportS3method tag.

26 changes: 19 additions & 7 deletions tests/testthat/test-namespace.R
Original file line number Diff line number Diff line change
Expand Up @@ -472,19 +472,31 @@ test_that("non-syntactic imports can use multiple quoting forms", {
# warn_missing_s3_exports -------------------------------------------------

test_that("warns if S3 method not documented", {
expect_snapshot(
roc_proc_text(namespace_roclet(), "
# Need to manually transform since the srcref is coming from the function;
# roc_proc_text() uses fake srcrefs for the blocks themselves
fix_srcref <- function(x) gsub("file[a-z0-9]+", "<text>", x)

block <- "
foo <- function(x) UseMethod('foo')
foo.numeric <- function(x) 1
mean.myclass <- function(x) 2
"),
# Need to manually transform since the srcref is coming from the function;
# roc_proc_text() uses fake srcrefs for the blocks themselves
transform = function(x) gsub("file[a-z0-9]+", "<text>", x)
"
expect_snapshot(
. <- roc_proc_text(namespace_roclet(), block),
transform = fix_srcref
)
})

# Works even if method contains {
block <- "
foo <- function(x) UseMethod('foo')
`foo.{` <- function(x) 1
"
expect_snapshot(
. <- roc_proc_text(namespace_roclet(), block),
transform = fix_srcref
)
})

test_that("can suppress the warning", {
block <- "
Expand Down

0 comments on commit 036e74a

Please sign in to comment.