Various code quality improvements (#1540)

* Don't change working directory in `load_package_copy()`. Fixes #1539. * Use `local_roxy_meta_set()` everywhere possible. Fixes #1536. * Reconsider display of warnings. Fixes #1533.
r-lib · Nov 21, 2023 · 30390aa · 30390aa
1 parent 56c5d5a
commit 30390aa
Show file tree

Hide file tree

Showing 49 changed files with 664 additions and 565 deletions.
diff --git a/R/block.R b/R/block.R
@@ -208,8 +208,7 @@ parse_tags <- function(tokens) {
   # Set up evaluation environment for markdown
   pkgenv <- roxy_meta_get("env") %||% baseenv()
   evalenv <- new.env(parent = pkgenv)
-  roxy_meta_set("evalenv", evalenv)
-  on.exit(roxy_meta_set("evalenv", NULL), add = TRUE)
+  local_roxy_meta_set("evalenv", evalenv)
 
   markdown_activate(tokens)
 
@@ -292,6 +291,7 @@ parse_description <- function(tags) {
 }
 
 warn_roxy_block <- function(block, message, ...) {
-  message[[1]] <- paste0(link_to(block$file, block$line), " ", message[[1]])
-  cli::cli_warn(message, ..., .envir = parent.frame())
+  message[[1]] <- paste0(link_to(block$file, block$line), ": ", message[[1]], ".")
+  names(message)[[1]] <- "x"
+  cli::cli_inform(message, ..., .envir = parent.frame())
 }
diff --git a/R/options.R b/R/options.R
@@ -145,8 +145,6 @@ roxy_meta_load <- function(base_path = getwd()) {
 }
 
 local_roxy_meta_set <- function(key, value, envir = caller_env()) {
-  old_value <- roxy_meta_get(key)
+  old_value <- roxy_meta_set(key, value)
   withr::defer(roxy_meta_set(key, old_value), envir = envir)
-
-  roxy_meta_set(key, value)
 }
diff --git a/R/roxygenize.R b/R/roxygenize.R
@@ -57,8 +57,7 @@ roxygenize <- function(package.dir = ".",
   # Now load code
   load_code <- find_load_strategy(load_code)
   env <- load_code(base_path)
-  roxy_meta_set("env", env)
-  on.exit(roxy_meta_set("env", NULL), add = TRUE)
+  local_roxy_meta_set("env", env)
 
   # Tokenise each file
   blocks <- parse_package(base_path, env = NULL)

diff --git a/R/tag.R b/R/tag.R
@@ -121,17 +121,18 @@ roxy_tag_warning <- function(x, ...) {
 #' @rdname roxy_tag
 warn_roxy_tag <- function(tag, message, ...) {
   message[[1]] <- paste0(
-    link_to(tag$file, tag$line), " @", tag$tag, " ",
+    link_to(tag$file, tag$line), ": {.strong @", tag$tag, "} ",
     if (is.null(tag$raw)) ("(automatically generated) "),
-    message[[1]]
+    message[[1]], "."
   )
-  cli::cli_warn(message, ..., .envir = parent.frame())
+  names(message)[[1]] <- "x"
+  cli::cli_inform(message, ..., .envir = parent.frame())
 }
 
 link_to <- function(file, line) {
-  paste0("[", cli::style_hyperlink(
+  cli::style_hyperlink(
     paste0(basename(file), ":", line),
     paste0("file://", file),
     params = c(line = line, col = 1)
-  ), "]")
+  )
 }
diff --git a/tests/testthat/_snaps/block.md b/tests/testthat/_snaps/block.md
@@ -1,33 +1,48 @@
 # has thoughtful print method
 
-    <roxy_block> [<text>:5]
-      $tag
-        [line:  1] @title 'This is a title' {parsed}
-        [line:  3] @param 'x,y A number' {parsed}
-        [line:  4] @export '' {parsed}
-        [line:  5] @usage '<generated>' {parsed}
-        [line:  5] @.formals '<generated>' {parsed}
-        [line:  5] @backref '<generated>' {parsed}
-      $call   f <- function(x, y) x + y
-      $object <function> 
-        $topic f
-        $alias f
+    Code
+      parse_text(text)[[1]]
+    Output
+      <roxy_block> [<text>:5]
+        $tag
+          [line:  1] @title 'This is a title' {parsed}
+          [line:  3] @param 'x,y A number' {parsed}
+          [line:  4] @export '' {parsed}
+          [line:  5] @usage '<generated>' {parsed}
+          [line:  5] @.formals '<generated>' {parsed}
+          [line:  5] @backref '<generated>' {parsed}
+        $call   f <- function(x, y) x + y
+        $object <function> 
+          $topic f
+          $alias f
 
 # errors are propagated
 
-    [<text>:5] @eval failed to evaluate
-    Caused by error in `foo()`:
-    ! Uhoh
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:5: @eval failed to evaluate.
+      Caused by error in `foo()`:
+      ! Uhoh
 
 # must return non-NA string
 
-    [<text>:3] @eval must evaluate to a character vector
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:3: @eval must evaluate to a character vector.
 
 ---
 
-    [<text>:3] @eval must not contain any missing values
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:3: @eval must not contain any missing values.
 
 # warns about duplicate tags
 
-    [<text>:5] Block must contain only one @rdname
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:5: Block must contain only one @rdname.
 
diff --git a/tests/testthat/_snaps/collate.md b/tests/testthat/_snaps/collate.md
@@ -9,10 +9,10 @@
 # DESCRIPTION file is re-written only if collate changes
 
     Code
-      update_collate(".")
+      update_collate(path)
     Message
-      Updating collate directive in './DESCRIPTION'
+      Updating collate directive in '<path>/DESCRIPTION'
     Code
       # Second run should be idempotent
-      update_collate(".")
+      update_collate(path)
 
diff --git a/tests/testthat/_snaps/markdown-code.md b/tests/testthat/_snaps/markdown-code.md
@@ -2,9 +2,8 @@
 
     Code
       out <- roc_proc_text(rd_roclet(), block)[[1]]
-    Condition
-      Warning:
-      [<text>:4] @description failed to evaluate inline markdown code
+    Message
+      x <text>:4: @description failed to evaluate inline markdown code.
       Caused by error:
       ! multi-line `r ` markup is not supported
 
@@ -17,9 +16,7 @@
     Message
       
       Quitting from lines 1-1
-    Condition
-      Warning:
-      [<text>:4] @description failed to evaluate inline markdown code
+      x <text>:4: @description failed to evaluate inline markdown code.
       Caused by error in `map_chr()`:
       i In index: 1.
       Caused by error:

diff --git a/tests/testthat/_snaps/markdown-link.md b/tests/testthat/_snaps/markdown-link.md
@@ -2,17 +2,15 @@
 
     Code
       markdown("[`foo` bar][x]", tag = tag)
-    Condition
-      Warning:
-      [foo.R:10] @title (automatically generated) markdown links must contain plain text
+    Message
+      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
+    Message
+      x foo.R:10: @title (automatically generated) markdown links must contain plain text.
       i Problematic link: x
     Output
       [1] ""
@@ -23,17 +21,18 @@
       out1 <- roc_proc_text(rd_roclet(),
       "\n    #' Title\n    #'\n    #' See [11pkg::function()], [11pkg::object].\n    #' @md\n    foo <- function() {}")[[
         1]]
-    Condition
-      Warning:
-      [<text>:4] @description refers to unavailable topic 11pkg::function
+    Message
+      x <text>:4: @description refers to unavailable topic 11pkg::function.
       Caused by error in `find.package()`:
       ! there is no package called '11pkg'
-      Warning:
-      [<text>:4] @description refers to unavailable topic 11pkg::object
+      x <text>:4: @description refers to unavailable topic 11pkg::object.
       Caused by error in `find.package()`:
       ! there is no package called '11pkg'
 
 ---
 
-    [<text>:4] @description refers to unavailable topic stringr::bar111
+    Code
+      out1 <- roc_proc_text(rd_roclet(), block)[[1]]
+    Message
+      x <text>:4: @description refers to unavailable topic stringr::bar111.
 
diff --git a/tests/testthat/_snaps/markdown-state.md b/tests/testthat/_snaps/markdown-state.md
@@ -1,4 +1,7 @@
 # warning for both @md and @noMd
 
-    [<text>:5] @md conflicts with @noMd; turning markdown parsing off
+    Code
+      out1 <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:5: @md conflicts with @noMd; turning markdown parsing off.
 
diff --git a/tests/testthat/_snaps/markdown.md b/tests/testthat/_snaps/markdown.md
@@ -76,14 +76,20 @@
 
 # unhandled markdown generates warning
 
-    [<text>:4] @description markdown translation failed
-    x block quotes are not currently supported
+    Code
+      . <- roc_proc_text(rd_roclet(), text)
+    Message
+      x <text>:4: @description markdown translation failed.
+      x block quotes are not currently supported
 
 # level 1 heading in markdown generates warning in some tags
 
-    [<text>:4] @seealso markdown translation failed
-    x Level 1 headings are not supported in @seealso
-    i Do you want to put the heading in @description or @details?
+    Code
+      . <- roc_proc_text(rd_roclet(), text)
+    Message
+      x <text>:4: @seealso markdown translation failed.
+      x Level 1 headings are not supported in @seealso
+      i Do you want to put the heading in @description or @details?
 
 # alternative knitr engines
 

diff --git a/tests/testthat/_snaps/namespace.md b/tests/testthat/_snaps/namespace.md
@@ -1,48 +1,94 @@
 # @exportS3method generates fully automatically
 
-    [<text>:2] @exportS3Method must be used with an known S3 method
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @exportS3Method must be used with an known S3 method.
 
 # @exportS3method can extract class from generic
 
-    [<text>:2] @exportS3Method must have form package::generic
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @exportS3Method must have form package::generic.
 
 ---
 
-    [<text>:2] @exportS3Method must be used with a function
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @exportS3Method must be used with a function.
 
 ---
 
-    [<text>:2] @exportS3Method doesn't match function name
-    x Expected to see "foo" to match "pkg::foo"
-    i Function name is "foo1.bar"
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @exportS3Method doesn't match function name.
+      x Expected to see "foo" to match "pkg::foo"
+      i Function name is "foo1.bar"
 
 # poorly formed importFrom throws error
 
-    [<text>:2] @importFrom must have at least 2 words, not 1
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @importFrom must have at least 2 words, not 1.
 
 # rawNamespace must be valid code
 
-    [<text>:2] @rawNamespace failed to parse
-    Caused by error in `parse()`:
-    ! <text>:2:0: unexpected end of input
-    1: a +
-       ^
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @rawNamespace failed to parse.
+      Caused by error in `parse()`:
+      ! <text>:2:0: unexpected end of input
+      1: a +
+         ^
 
 # evalNamespace warns for bad code
 
-    [<text>:2] @evalNamespace failed to parse
-    Caused by error in `parse()`:
-    ! <text>:2:0: unexpected end of input
-    1: a +
-       ^
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @evalNamespace failed to parse.
+      Caused by error in `parse()`:
+      ! <text>:2:0: unexpected end of input
+      1: a +
+         ^
 
 ---
 
-    [<text>:2] @evalNamespace failed to evaluate
-    Caused by error:
-    ! Uhoh
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @evalNamespace failed to evaluate.
+      Caused by error:
+      ! Uhoh
 
 ---
 
-    [<text>:2] @evalNamespace must evaluate to a character vector
+    Code
+      . <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @evalNamespace must evaluate to a character vector.
+
+# Invalid imports throw a helpful error
+
+    Code
+      out <- roc_proc_text(namespace_roclet(), block)
+    Message
+      x <text>:2: @importFrom Excluding unknown export in 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`.
+
+---
+
+    Code
+      out <- roc_proc_text(namespace_roclet(), block)
 
diff --git a/tests/testthat/_snaps/rd-describe-in.md b/tests/testthat/_snaps/rd-describe-in.md
@@ -111,13 +111,22 @@
 
 # complains about bad usage
 
-    [<text>:6] @describeIn must be used with an object
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:6: @describeIn must be used with an object.
 
 ---
 
-    [<text>:6] @describeIn can not be used with @name
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:6: @describeIn can not be used with @name.
 
 ---
 
-    [<text>:6] @describeIn can not be used with @rdname
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:6: @describeIn can not be used with @rdname.