Use new warn_roxy_tag() and warn_roxy_block() (#1317)

* Implement `warn_roxy_tag()` and use instead of `roxy_tag_warning()`.
* Markdown parser always checks for matched `{`
* Code is parsed once and recorded in `tag_code()`
* New get `roxy_generated_tag()` helper that takes a block
* File name resolution happens in `rd_section_reexport()` so we can use consistent warnings (always mentioning the tag). All tags now have file and line information, even if it's for the block.

Author: hadley

NAMESPACE

@@ -238,6 +238,7 @@ export(tag_words)
 export(tag_words_line)
 export(update_collate)
 export(vignette_roclet)
+export(warn_roxy_tag)
 import(rlang)
 import(stringr)
 importFrom(R6,R6Class)

NEWS.md

@@ -1,5 +1,8 @@
 # roxygen2 (development version)
 
+* All warning messages have been reviewed to hopefully be more informative
+  and actionable (#1317).
+
 * DOIs, arXiv links, and urls in the `Description` field of the `DESCRIPTION`
   are now converted to the appropriate Rd markup (@dieghernan, #1265, #1164).
 

R/block.R

@@ -143,8 +143,8 @@ block_find_object <- function(block, env) {
   ))
 
   # Add in defaults generated from the object
-  defaults <- object_defaults(object)
-  defaults <- c(defaults, list(roxy_tag("backref", block$file, block$file)))
+  defaults <- object_defaults(object, block)
+  defaults <- c(defaults, list(roxy_generated_tag(block, "backref", block$file)))
 
   default_tags <- map_chr(defaults, "tag")
   defaults <- defaults[!default_tags %in% block_tags(block)]
@@ -181,7 +181,7 @@ block_get_tag <- function(block, tag) {
   } else if (n == 1) {
     block$tags[[matches]]
   } else {
-    roxy_tag_warning(block$tags[[matches[[2]]]], "May only use one @", tag, " per block")
+    warn_roxy_block(block, "Block must contain only one @{tag}")
     block$tags[[matches[[1]]]]
   }
 }
@@ -289,3 +289,8 @@ parse_description <- function(tags) {
 
   c(compact(list(title, description, details)), tags)
 }
+
+warn_roxy_block <- function(block, message, ...) {
+  message[[1]] <- paste0("[", block$file, ":", block$line, "] ", message[[1]])
+  cli::cli_warn(message, ..., .envir = parent.frame())
+}

R/markdown-link.R

@@ -105,9 +105,10 @@ parse_link <- function(destination, contents, state) {
   if (!all(xml_name(contents) %in% c("text", "softbreak", "linebreak"))) {
     incorrect <- setdiff(unique(xml_name(contents)), c("text", "softbreak", "linebreak"))
 
-    roxy_tag_warning(state$tag,
-      "Links must contain plain text. Problematic link: ", destination
-    )
+    warn_roxy_tag(state$tag, c(
+      "markdown links must contain plain text",
+      i = "Problematic link: {destination}"
+    ))
     return("")
   }
 

R/markdown-state.R

@@ -16,6 +16,12 @@ markdown_on <- function(value = NULL) {
   return(isTRUE(markdown_env$`markdown-support`))
 }
 
+local_markdown <- function(env = parent.frame()) {
+  old <- markdown_env$`markdown-support`
+  markdown_on(TRUE)
+  withr::defer(markdown_on(old), envir = env)
+}
+
 markdown_activate <- function(tags) {
   ## markdown on/off based on global flag and presence of @md & @nomd
 
@@ -24,12 +30,15 @@ markdown_activate <- function(tags) {
   has_nomd <- "noMd" %in% names
 
   if (has_md && has_nomd) {
-    roxy_tag_warning(tags[[1]], "Both @md and @noMd, no markdown parsing")
+    md_tag <- tags[names == "md"][[1]]
+    warn_roxy_tag(md_tag, "conflicts with @noMd; turning markdown parsing off")
+
+    md <- FALSE
+  } else {
+    md <- roxy_meta_get("markdown", FALSE)
+    if (has_md) md <- TRUE
+    if (has_nomd) md <- FALSE
   }
 
-  md <- roxy_meta_get("markdown", FALSE)
-  if (has_md) md <- TRUE
-  if (has_nomd) md <- FALSE
-
   markdown_on(md)
 }

R/markdown.R

@@ -183,7 +183,11 @@ mdxml_children_to_rd <- function(xml, state) {
 mdxml_node_to_rd <- function(xml, state) {
   if (!inherits(xml, "xml_node") ||
       ! xml_type(xml) %in% c("text", "element")) {
-    roxy_tag_warning(state$tag, "Internal markdown translation failure")
+    warn_roxy_tag(state$tag, c(
+      "markdown translation failed",
+      x = "Unexpected internal error",
+      i = "Please file an issue at https://github.com/r-lib/roxygen2/issues"
+    ))
     return("")
   }
 
@@ -221,11 +225,18 @@ mdxml_node_to_rd <- function(xml, state) {
 }
 
 mdxml_unknown <- function(xml, tag) {
-  roxy_tag_warning(tag, "Unknown xml node: ", xml_name(xml))
+  warn_roxy_tag(tag, c(
+    "markdown translation failed",
+    x = "Internal error: unknown xml node {xml_name(xml)}",
+    i = "Please file an issue at https://github.com/r-lib/roxygen2/issues"
+  ))
   escape_comment(xml_text(xml))
 }
 mdxml_unsupported <- function(xml, tag, feature) {
-  roxy_tag_warning(tag, "Use of ", feature, " is not currently supported")
+  warn_roxy_tag(tag, c(
+    "markdown translation failed",
+    x = "{feature} are not currently supported"
+  ))
   escape_comment(xml_text(xml))
 }
 
@@ -398,7 +409,7 @@ mdxml_html_block <- function(xml, state) {
 
 mdxml_html_inline <- function(xml, state) {
   if (state$tag$tag != "includeRmd") {
-    return(mdxml_unsupported(xml, state$tag, "inline HTML"))
+    return(mdxml_unsupported(xml, state$tag, "inline HTML components"))
   }
   paste0(
     "\\if{html}{\\out{",

R/namespace.R

@@ -175,9 +175,7 @@ roxy_tag_ns.roxy_tag_exportS3Method <- function(x, block, env, import_only = FAL
 
   obj <- block$object
   if (length(x$val) < 2 && !inherits(obj, "s3method")) {
-    roxy_tag_warning(x,
-      "`@exportS3Method` and `@exportS3Method generic` must be used with an S3 method"
-    )
+    warn_roxy_tag(x, "must be used with an S3 method when it has 0 or 1 values")
     return()
   }
 
@@ -234,7 +232,7 @@ roxy_tag_parse.roxy_tag_rawNamespace <- function(x) {
 }
 #' @export
 roxy_tag_ns.roxy_tag_rawNamespace  <- function(x, block, env, import_only = FALSE) {
-  x$val
+  x$raw
 }
 
 #' @export

R/object-defaults.R

@@ -1,14 +1,14 @@
-object_defaults <- function(x) UseMethod("object_defaults")
+object_defaults <- function(x, block) UseMethod("object_defaults")
 
 #' @export
-object_defaults.default <- function(x) list()
+object_defaults.default <- function(x, block) list()
 
 #' @exportS3Method object_defaults "function"
-object_defaults.function <- function(x) {
+object_defaults.function <- function(x, block) {
   list(
-    roxy_tag("usage", NULL, object_usage(x)),
+    roxy_generated_tag(block, "usage", object_usage(x)),
     # Used in process_inherit_params()
-    roxy_tag(".formals", NULL, names(formals(x$value)))
+    roxy_generated_tag(block, ".formals", names(formals(x$value)))
   )
 }
 
@@ -25,19 +25,19 @@ object_defaults.s4generic <- object_defaults.function
 object_defaults.s4method <- object_defaults.function
 
 #' @export
-object_defaults.data <- function(x) {
+object_defaults.data <- function(x, block) {
   str_out <- rd(object_format(x$value))
 
   list(
-    roxy_tag("docType", NULL, "data"),
-    roxy_tag("format", NULL, str_out),
-    roxy_tag("keywords", NULL, "datasets"),
-    roxy_tag("usage", NULL, object_usage(x))
+    roxy_generated_tag(block, "docType", "data"),
+    roxy_generated_tag(block, "format", str_out),
+    roxy_generated_tag(block, "keywords", "datasets"),
+    roxy_generated_tag(block, "usage", object_usage(x))
   )
 }
 
 #' @export
-object_defaults.package <- function(x) {
+object_defaults.package <- function(x, block) {
   desc <- x$value$desc
 
   description <- as.character(desc$Description)
@@ -49,41 +49,41 @@ object_defaults.package <- function(x) {
   }
 
   list(
-    roxy_tag("docType", NULL, "package"),
-    roxy_tag("name", NULL, package_suffix(desc$Package)),
+    roxy_generated_tag(block, "docType", "package"),
+    roxy_generated_tag(block, "name", package_suffix(desc$Package)),
     # "NULL" prevents addition of default aliases, see also #202
-    roxy_tag("aliases", NULL, paste("NULL", desc$Package, package_suffix(desc$Package))),
-    roxy_tag("title", NULL, paste0(desc$Package, ": ", desc$Title)),
-    roxy_tag("description", NULL, description),
-    roxy_tag("seealso", NULL, package_seealso(desc)),
-    roxy_tag("author", NULL, package_authors(desc))
+    roxy_generated_tag(block, "aliases", paste("NULL", desc$Package, package_suffix(desc$Package))),
+    roxy_generated_tag(block, "title", paste0(desc$Package, ": ", desc$Title)),
+    roxy_generated_tag(block, "description", description),
+    roxy_generated_tag(block, "seealso", package_seealso(desc)),
+    roxy_generated_tag(block, "author", package_authors(desc))
   )
 }
 
 #' @export
-object_defaults.import <- function(x) {
+object_defaults.import <- function(x, block) {
   list(
-    roxy_tag("docType", NULL, "import"),
-    roxy_tag("name", NULL, "reexports"),
-    roxy_tag("keywords", NULL, "internal"),
-    roxy_tag("title", NULL, "Objects exported from other packages"),
-    roxy_tag(".reexport", NULL, list(pkg = x$value$pkg, fun = x$value$fun))
+    roxy_generated_tag(block, "docType", "import"),
+    roxy_generated_tag(block, "name", "reexports"),
+    roxy_generated_tag(block, "keywords", "internal"),
+    roxy_generated_tag(block, "title", "Objects exported from other packages"),
+    roxy_generated_tag(block, ".reexport", list(pkg = x$value$pkg, fun = x$value$fun))
   )
 }
 
 #' @export
-object_defaults.s4class <- function(x) {
+object_defaults.s4class <- function(x, block) {
   list(
-    roxy_tag("docType", NULL, "class")
+    roxy_generated_tag(block, "docType", "class")
   )
 }
 
 #' @export
-object_defaults.rcclass <- function(x) {
+object_defaults.rcclass <- function(x, block) {
   list(
-    roxy_tag("docType", NULL, "class"),
+    roxy_generated_tag(block, "docType", "class"),
     if (!is.null(x$methods))
-      roxy_tag(".methods", NULL, x$methods)
+      roxy_generated_tag(block, ".methods", x$methods)
   )
 }
 

R/object-import.R

@@ -1,37 +1,44 @@
 # Re-export ----------------------------------------------------------------
-rd_section_reexport <- function(pkg, fun) {
-  stopifnot(is.character(pkg), is.character(fun))
+rd_section_reexport <- function(pkg, fun, file) {
+  stopifnot(is.character(pkg), is.character(fun), is.character(file))
   stopifnot(length(pkg) == length(fun))
 
-  rd_section("reexport", list(pkg = pkg, fun = fun))
+  rd_section("reexport", list(pkg = pkg, fun = fun, file = file))
 }
 
 #' @export
 roxy_tag_rd.roxy_tag_.reexport <- function(x, base_path, env) {
-  rd_section_reexport(x$val$pkg, x$val$fun)
+  file <- find_topic_filename(x$val$pkg, x$val$fun, tag = x)
+  rd_section_reexport(x$val$pkg, x$val$fun, file)
 }
 #' @export
 merge.rd_section_reexport <- function(x, y, ...) {
   stopifnot(identical(class(x), class(y)))
-  rd_section_reexport(c(x$value$pkg, y$value$pkg), c(x$value$fun, y$value$fun))
+  rd_section_reexport(
+    c(x$value$pkg, y$value$pkg),
+    c(x$value$fun, y$value$fun),
+    c(x$value$file, y$value$file)
+  )
 }
 #' @export
 format.rd_section_reexport <- function(x, ...) {
-  pkgs <- split(x$value$fun, x$value$pkg)
-  pkg_links <- map2(names(pkgs), pkgs, function(pkg, funs) {
-    funs <- sort_c(funs)
-    files <- map_chr(
-      funs,
-      try_find_topic_in_package,
-      pkg = pkg,
-      where = " in re-export"
-    )
+
+  info <- data.frame(
+    pkg = x$value$pkg,
+    fun = x$value$fun,
+    file = x$value$file,
+    stringsAsFactors = FALSE
+  )
+
+  pkgs <- split(info, x$value$pkg)
+  pkg_links <- map(pkgs, function(pkg) {
+    pkg <- pkg[order(pkg$fun), ]
     links <- paste0(
-      "\\code{\\link[", pkg,
-      ifelse(files == funs, "", paste0(":", files)),
-      "]{", escape(funs), "}}",
+      "\\code{\\link[", pkg$pkg,
+      ifelse(pkg$file == pkg$fun, "", paste0(":", pkg$file)),
+      "]{", escape(pkg$fun), "}}",
       collapse = ", ")
-    paste0("\\item{", pkg, "}{", links, "}")
+    paste0("\\item{", pkg$pkg, "}{", links, "}")
   })
 
   paste0(

R/object-r6.R

@@ -1,10 +1,10 @@
 #' @export
-object_defaults.r6class <- function(x) {
+object_defaults.r6class <- function(x, block) {
   r6on <- roxy_meta_get("r6", TRUE)
   if (isTRUE(r6on)) {
     list(
-      roxy_tag("docType", NULL, NULL),
-      roxy_tag(".r6data", NULL, extract_r6_data(x$value))
+      roxy_generated_tag(block, "docType", NULL),
+      roxy_generated_tag(block, ".r6data", extract_r6_data(x$value))
     )
   } else {
     NextMethod()