From c537e9fef63377264d87c31ced6802b809c9c0de Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Tue, 21 Nov 2023 16:26:49 -0600
Subject: [PATCH] Improve messages for two part tags

And provide a helpful suggestion for `@describeIn`. Fixes #1493.
---
 NEWS.md                                 |  3 +++
 R/rd-describe-in.R                      | 10 +++++++++-
 R/rd-include-rmd.R                      |  6 +++---
 R/rd-inherit.R                          |  8 +++++---
 R/rd-params.R                           |  2 +-
 R/rd-s4.R                               |  8 ++++++--
 R/rd-template.R                         |  2 +-
 R/tag-parser.R                          | 12 ++++++++----
 tests/testthat/_snaps/rd-describe-in.md |  8 ++++++++
 tests/testthat/_snaps/rd-include-rmd.md |  7 +++++++
 tests/testthat/_snaps/rd-inherit.md     |  8 ++++++++
 tests/testthat/_snaps/rd-params.md      |  2 +-
 tests/testthat/_snaps/rd-s4.md          |  8 ++++++++
 tests/testthat/_snaps/rd-template.md    |  7 +++++++
 tests/testthat/_snaps/tag-parser.md     | 20 ++++++++++----------
 tests/testthat/test-rd-describe-in.R    |  9 +++++++++
 tests/testthat/test-rd-include-rmd.R    |  8 ++++++++
 tests/testthat/test-rd-inherit.R        |  9 +++++++++
 tests/testthat/test-rd-s4.R             | 11 +++++++++++
 tests/testthat/test-rd-template.R       | 10 ++++++++++
 tests/testthat/test-tag-parser.R        |  6 +++---
 21 files changed, 135 insertions(+), 29 deletions(-)
 create mode 100644 tests/testthat/_snaps/rd-s4.md

diff --git a/NEWS.md b/NEWS.md
index ee6a4a520..5a8c6b580 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,8 @@
 # roxygen2 (development version)
 
+* `@describeIn foo` now suggests that you might want `@rdname` instead 
+  (#1493).
+
 * If you document a function from another package it is automatically 
   imported. Additionally, if you set `@rdname` or `@name` you can opt out 
   of the default `reexports` topic generation and provide your own docs 
diff --git a/R/rd-describe-in.R b/R/rd-describe-in.R
index 0d9f2013f..fc305897c 100644
--- a/R/rd-describe-in.R
+++ b/R/rd-describe-in.R
@@ -1,6 +1,14 @@
 #' @export
 roxy_tag_parse.roxy_tag_describeIn <- function(x) {
-  tag_name_description(x)
+  if (!is.na(x$raw) && !str_detect(x$raw, "[[:space:]]+")) {
+    warn_roxy_tag(x, c(
+      "requires a name and description",
+      i = "Did you want @rdname instead?"
+    ))
+    NULL
+  } else {
+    tag_two_part(x, "a topic name", "a description")
+  }
 }
 
 topic_add_describe_in <- function(topic, block, env) {
diff --git a/R/rd-include-rmd.R b/R/rd-include-rmd.R
index 04f0c9707..0624cfbaa 100644
--- a/R/rd-include-rmd.R
+++ b/R/rd-include-rmd.R
@@ -5,13 +5,13 @@ roxy_tag_parse.roxy_tag_includeRmd <- function(x) {
     return()
   }
 
-  tag_two_part(x, "path", "section", required = FALSE, markdown = FALSE)
+  tag_two_part(x, "a path", "a section", required = FALSE, markdown = FALSE)
 }
 
 #' @export
 roxy_tag_rd.roxy_tag_includeRmd <- function(x, base_path, env) {
-  rmd <- rel_rmd <- x$val$path
-  section <- x$val$section
+  rmd <- rel_rmd <- x$val$name
+  section <- x$val$description
 
   if (!file.exists(rmd)) {
     warn_roxy_tag(x, "Can't find Rmd {.path {rmd}}")
diff --git a/R/rd-inherit.R b/R/rd-inherit.R
index 10a2ed2d5..e508e3b1f 100644
--- a/R/rd-inherit.R
+++ b/R/rd-inherit.R
@@ -16,15 +16,17 @@ roxy_tag_rd.roxy_tag_inheritParams <- function(x, base_path, env) {
 
 #' @export
 roxy_tag_parse.roxy_tag_inheritDotParams <- function(x) {
-  tag_two_part(x, "source", "args", required = FALSE, markdown = FALSE)
+  tag_two_part(x, "a source", "an argument list", required = FALSE, markdown = FALSE)
 }
 #' @export
 roxy_tag_rd.roxy_tag_inheritDotParams <- function(x, base_path, env) {
-  rd_section_inherit_dot_params(x$val$source, x$val$args)
+  rd_section_inherit_dot_params(x$val$name, x$val$description)
 }
 
 #' @export
-roxy_tag_parse.roxy_tag_inheritSection <- function(x) tag_name_description(x)
+roxy_tag_parse.roxy_tag_inheritSection <- function(x) {
+  tag_two_part(x, "a topic name", "a section title")
+}
 #' @export
 roxy_tag_rd.roxy_tag_inheritSection <- function(x, base_path, env) {
   rd_section_inherit_section(x$val$name, x$val$description)
diff --git a/R/rd-params.R b/R/rd-params.R
index 57c4906aa..6bcb099cf 100644
--- a/R/rd-params.R
+++ b/R/rd-params.R
@@ -1,6 +1,6 @@
 #' @export
 roxy_tag_parse.roxy_tag_param <- function(x) {
-  tag_name_description(x)
+  tag_two_part(x, "an argument name", "a description")
 }
 
 #' @export
diff --git a/R/rd-s4.R b/R/rd-s4.R
index 2c96fb015..faeb3333b 100644
--- a/R/rd-s4.R
+++ b/R/rd-s4.R
@@ -1,5 +1,7 @@
 #' @export
-roxy_tag_parse.roxy_tag_field <- function(x) tag_name_description(x)
+roxy_tag_parse.roxy_tag_field <- function(x) {
+  tag_two_part(x, "a field name", "a description")
+}
 #' @export
 roxy_tag_rd.roxy_tag_field <- function(x, base_path, env) {
   value <- setNames(x$val$description, x$val$name)
@@ -11,7 +13,9 @@ format.rd_section_field <- function(x, ...) {
 }
 
 #' @export
-roxy_tag_parse.roxy_tag_slot <- function(x) tag_name_description(x)
+roxy_tag_parse.roxy_tag_slot <- function(x) {
+  tag_two_part(x, "a slot name", "a description")
+}
 #' @export
 roxy_tag_rd.roxy_tag_slot <- function(x, base_path, env) {
   value <- setNames(x$val$description, x$val$name)
diff --git a/R/rd-template.R b/R/rd-template.R
index 3c6aecbb1..9828c9170 100644
--- a/R/rd-template.R
+++ b/R/rd-template.R
@@ -5,7 +5,7 @@ roxy_tag_parse.roxy_tag_template <- function(x) {
 
 #' @export
 roxy_tag_parse.roxy_tag_templateVar <- function(x) {
-  tag_name_description(x)
+  tag_two_part(x, "a variable name", "a value")
 }
 
 process_templates <- function(block, base_path) {
diff --git a/R/tag-parser.R b/R/tag-parser.R
index 37465a269..9a08dbf56 100644
--- a/R/tag-parser.R
+++ b/R/tag-parser.R
@@ -98,10 +98,14 @@ tag_name <- function(x) {
 #' @param markdown Should the second part be parsed as markdown?
 tag_two_part <- function(x, first, second, required = TRUE, markdown = TRUE) {
   if (str_trim(x$raw) == "") {
-    warn_roxy_tag(x, "requires a value")
+    if (!required) {
+      warn_roxy_tag(x, "requires {first}")
+    } else {
+      warn_roxy_tag(x, "requires two parts: {first} and {second}")
+    }
     NULL
   } else if (required && !str_detect(x$raw, "[[:space:]]+")) {
-    warn_roxy_tag(x, "requires {first} and {second}")
+    warn_roxy_tag(x, "requires two parts: {first} and {second}")
     NULL
   } else if (!rdComplete(x$raw, is_code = FALSE)) {
     warn_roxy_tag(x, "has mismatched braces or quotes")
@@ -118,7 +122,7 @@ tag_two_part <- function(x, first, second, required = TRUE, markdown = TRUE) {
       pieces[, 1],
       trim_docstring(pieces[,2])
     )
-    names(x$val) <- c(first, second)
+    names(x$val) <- c("name", "description")
     x
   }
 }
@@ -126,7 +130,7 @@ tag_two_part <- function(x, first, second, required = TRUE, markdown = TRUE) {
 #' @export
 #' @rdname tag_parsers
 tag_name_description <- function(x) {
-  tag_two_part(x, "name", "description")
+  tag_two_part(x, "a name", "a description")
 }
 
 #' @export
diff --git a/tests/testthat/_snaps/rd-describe-in.md b/tests/testthat/_snaps/rd-describe-in.md
index 53522c110..985ef28f4 100644
--- a/tests/testthat/_snaps/rd-describe-in.md
+++ b/tests/testthat/_snaps/rd-describe-in.md
@@ -1,3 +1,11 @@
+# @describeIn suggests @rdname
+
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:2: @describeIn requires a name and description.
+      i Did you want @rdname instead?
+
 # Multiple @describeIn functions combined into one
 
     Code
diff --git a/tests/testthat/_snaps/rd-include-rmd.md b/tests/testthat/_snaps/rd-include-rmd.md
index 1a5f140fc..5d4a31276 100644
--- a/tests/testthat/_snaps/rd-include-rmd.md
+++ b/tests/testthat/_snaps/rd-include-rmd.md
@@ -1,3 +1,10 @@
+# invalid syntax gives useful warning
+
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:2: @includeRmd requires a path.
+
 # useful warnings
 
     Code
diff --git a/tests/testthat/_snaps/rd-inherit.md b/tests/testthat/_snaps/rd-inherit.md
index 520805177..29bedefe2 100644
--- a/tests/testthat/_snaps/rd-inherit.md
+++ b/tests/testthat/_snaps/rd-inherit.md
@@ -5,6 +5,14 @@
     "sha1"}
     } 
 
+# invalid syntax gives useful warning
+
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:2: @inheritDotParams requires a source.
+      x <text>:3: @inheritSection requires two parts: a topic name and a section title.
+
 # warns on unknown inherit type
 
     Code
diff --git a/tests/testthat/_snaps/rd-params.md b/tests/testthat/_snaps/rd-params.md
index 60ff5f02c..11fb86ebe 100644
--- a/tests/testthat/_snaps/rd-params.md
+++ b/tests/testthat/_snaps/rd-params.md
@@ -3,5 +3,5 @@
     Code
       . <- roc_proc_text(rd_roclet(), block)
     Message
-      x <text>:3: @param requires a value.
+      x <text>:3: @param requires two parts: an argument name and a description.
 
diff --git a/tests/testthat/_snaps/rd-s4.md b/tests/testthat/_snaps/rd-s4.md
new file mode 100644
index 000000000..6760df286
--- /dev/null
+++ b/tests/testthat/_snaps/rd-s4.md
@@ -0,0 +1,8 @@
+# invalid syntax generates useful warning
+
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:3: @field requires two parts: a field name and a description.
+      x <text>:4: @slot requires two parts: a slot name and a description.
+
diff --git a/tests/testthat/_snaps/rd-template.md b/tests/testthat/_snaps/rd-template.md
index 32857ff8d..a5882823c 100644
--- a/tests/testthat/_snaps/rd-template.md
+++ b/tests/testthat/_snaps/rd-template.md
@@ -1,3 +1,10 @@
+# invalid syntax generates useful warning
+
+    Code
+      . <- roc_proc_text(rd_roclet(), block)
+    Message
+      x <text>:3: @templateVar requires two parts: a variable name and a value.
+
 # templates gives useful error if not found
 
     Code
diff --git a/tests/testthat/_snaps/tag-parser.md b/tests/testthat/_snaps/tag-parser.md
index 3794bea83..c732e331d 100644
--- a/tests/testthat/_snaps/tag-parser.md
+++ b/tests/testthat/_snaps/tag-parser.md
@@ -19,18 +19,12 @@
       <message/rlang_message>
       Message:
       x test.R:1: @test requires a value.
-    Code
-      expect_parse_failure(tag_two_part(tag))
-    Output
-      <message/rlang_message>
-      Message:
-      x test.R:1: @test requires a value.
     Code
       expect_parse_failure(tag_name_description(tag))
     Output
       <message/rlang_message>
       Message:
-      x test.R:1: @test requires a value.
+      x test.R:1: @test requires two parts: a name and a description.
     Code
       expect_parse_failure(tag_code(tag))
     Output
@@ -146,12 +140,18 @@
 # tag_two_part() gives useful warnings
 
     Code
-      tag <- roxy_test_tag("a")
-      expect_parse_failure(tag_two_part(tag, "name", "value"))
+      tag <- roxy_test_tag("")
+      expect_parse_failure(tag_two_part(tag, "a name", "a value", required = FALSE))
+    Output
+      <message/rlang_message>
+      Message:
+      x test.R:1: @test requires a name.
+    Code
+      expect_parse_failure(tag_two_part(tag, "a name", "a value"))
     Output
       <message/rlang_message>
       Message:
-      x test.R:1: @test requires name and value.
+      x test.R:1: @test requires two parts: a name and a value.
 
 # tag_words() gives useful warnings
 
diff --git a/tests/testthat/test-rd-describe-in.R b/tests/testthat/test-rd-describe-in.R
index 46b4fa413..3fca03ecd 100644
--- a/tests/testthat/test-rd-describe-in.R
+++ b/tests/testthat/test-rd-describe-in.R
@@ -1,3 +1,11 @@
+test_that("@describeIn suggests @rdname", {
+  block <- "
+    #' @describeIn foo
+    NULL
+  "
+  expect_snapshot(. <- roc_proc_text(rd_roclet(), block))
+})
+
 test_that("@describeIn generic destination captures s3 method source", {
   out <- roc_proc_text(rd_roclet(), "
     #' Title
@@ -252,3 +260,4 @@ test_that("complains about bad usage", {
   "
   expect_snapshot(. <- roc_proc_text(rd_roclet(), block))
 })
+
diff --git a/tests/testthat/test-rd-include-rmd.R b/tests/testthat/test-rd-include-rmd.R
index ff6e7064a..8b26cd626 100644
--- a/tests/testthat/test-rd-include-rmd.R
+++ b/tests/testthat/test-rd-include-rmd.R
@@ -1,5 +1,13 @@
 skip_if_not_installed("rmarkdown")
 
+test_that("invalid syntax gives useful warning", {
+  block <- "
+    #' @includeRmd
+    NULL
+  "
+  expect_snapshot(. <- roc_proc_text(rd_roclet(), block))
+})
+
 test_that("markdown file can be included", {
   skip_if_not(rmarkdown::pandoc_available("2.17"))
 
diff --git a/tests/testthat/test-rd-inherit.R b/tests/testthat/test-rd-inherit.R
index bcefb356e..0f8883c09 100644
--- a/tests/testthat/test-rd-inherit.R
+++ b/tests/testthat/test-rd-inherit.R
@@ -59,6 +59,15 @@ test_that("relative links converted to absolute", {
 
 # tag parsing -------------------------------------------------------------
 
+test_that("invalid syntax gives useful warning", {
+  block <- "
+    #' @inheritDotParams
+    #' @inheritSection
+    NULL
+  "
+  expect_snapshot(. <- roc_proc_text(rd_roclet(), block))
+})
+
 test_that("warns on unknown inherit type", {
   text <- "
     #' @inherit fun blah
diff --git a/tests/testthat/test-rd-s4.R b/tests/testthat/test-rd-s4.R
index 6fd25fddd..d0cc1bee1 100644
--- a/tests/testthat/test-rd-s4.R
+++ b/tests/testthat/test-rd-s4.R
@@ -1,3 +1,14 @@
+test_that("invalid syntax generates useful warning", {
+  block <- "
+    #' A
+    #' @field
+    #' @slot
+    a <- function() {}
+  "
+
+  expect_snapshot(. <- roc_proc_text(rd_roclet(), block))
+})
+
 test_that("@fields creates a new section and lists fields", {
   out <- roc_proc_text(rd_roclet(), "
     #' Important class.
diff --git a/tests/testthat/test-rd-template.R b/tests/testthat/test-rd-template.R
index c0da82b8a..5c001fc9a 100644
--- a/tests/testthat/test-rd-template.R
+++ b/tests/testthat/test-rd-template.R
@@ -1,3 +1,13 @@
+test_that("invalid syntax generates useful warning", {
+  block <- "
+    #' A
+    #' @templateVar
+    a <- function() {}
+  "
+
+  expect_snapshot(. <- roc_proc_text(rd_roclet(), block))
+})
+
 test_that("can find template from name", {
   base <- test_path("templates/")
 
diff --git a/tests/testthat/test-tag-parser.R b/tests/testthat/test-tag-parser.R
index 10448a006..02df85037 100644
--- a/tests/testthat/test-tag-parser.R
+++ b/tests/testthat/test-tag-parser.R
@@ -4,7 +4,6 @@ test_that("tags containing only whitespace generate warning", {
     expect_parse_failure(tag_value(tag))
     expect_parse_failure(tag_inherit(tag))
     expect_parse_failure(tag_name(tag))
-    expect_parse_failure(tag_two_part(tag))
     expect_parse_failure(tag_name_description(tag))
     expect_parse_failure(tag_code(tag))
     expect_parse_failure(tag_examples(tag))
@@ -54,8 +53,9 @@ test_that("tag_name() checks for valid names", {
 test_that("tag_two_part() gives useful warnings", {
   local_markdown()
   expect_snapshot({
-    tag <- roxy_test_tag("a")
-    expect_parse_failure(tag_two_part(tag, "name", "value"))
+    tag <- roxy_test_tag("")
+    expect_parse_failure(tag_two_part(tag, "a name", "a value", required = FALSE))
+    expect_parse_failure(tag_two_part(tag, "a name", "a value"))
   })
 })