From 0c16c8fbecc40c84715fc3c8c35d3bfc743d3a0f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 1 Apr 2022 14:54:03 -0500 Subject: [PATCH] Inherit group params exactly (#1316) Fixes #950 --- NEWS.md | 4 ++ R/rd-inherit.R | 75 ++++++++++++-------------------- tests/testthat/test-rd-inherit.R | 56 ++++++++++++++++-------- 3 files changed, 71 insertions(+), 64 deletions(-) diff --git a/NEWS.md b/NEWS.md index c892e970..226f3e94 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,9 @@ # roxygen2 (development version) +* `@inheritParams` now only inherits exact multiparameter matches, so if you're + inheriting from a function with `@param x,y` you'll only get the parameter + documentation if your function needs docs for both x and y (#950). + * All tags warn now if you only provide whitespace (#1228). * Add support for inheriting 'note' fields via `@inherit pkg::fun note` (@pat-s, #1218) diff --git a/R/rd-inherit.R b/R/rd-inherit.R index 391fbcff..a34a8df3 100644 --- a/R/rd-inherit.R +++ b/R/rd-inherit.R @@ -141,26 +141,28 @@ inherit_params <- function(topic, topics) { } for (inheritor in inheritors) { - inherited <- find_params(inheritor, topics) - - matches <- map_chr(missing, match_param, names(inherited)) - new_match <- !is.na(matches) - - if (!any(new_match)) { - # Can't warn here because @inherit inherits parameters - next + inherited_params <- find_params(inheritor, topics) + + for (param in inherited_params) { + match <- match_param(param$name, missing) + if (all(!is.na(match))) { + param_val <- setNames(param$value, paste(match, collapse = ",")) + topic$add(rd_section("param", param_val)) + missing <- setdiff(missing, match) + } } - - topic$add( - rd_section( - "param", - setNames(inherited[matches[new_match]], missing[new_match]) - ) - ) - missing <- missing[!new_match] } } +# Ignore . prefix since it's sometimes necessary to add because a +# function uses ... +match_param <- function(needle, haystack) { + needle_std <- gsub("^\\.", "", needle) + haystack_std <- gsub("^\\.", "", haystack) + + haystack[match(needle_std, haystack_std)] +} + inherit_dot_params <- function(topic, topics, env) { inheritors <- topic$get_value("inherit_dot_params") if (is.null(inheritors)) @@ -173,9 +175,12 @@ inherit_dot_params <- function(topic, topics, env) { # Then pull out the ones we need docs <- lapply(inheritors$source, find_params, topics = topics) arg_matches <- function(args, docs) { - doc_args <- str_split(names(docs), ", ?") - match <- map_lgl(doc_args, function(x) x %in% args) - docs[match] + match <- map_lgl(docs, function(x) x$name %in% args) + matched <- docs[match] + setNames( + lapply(matched, "[[", "value"), + map_chr(matched, function(x) paste(x$name, collapse = ",")) + ) } docs_selected <- unlist(map2(args, docs, arg_matches)) @@ -232,11 +237,10 @@ find_params <- function(name, topics) { param_names <- str_trim(names(params)) param_names[param_names == "\\dots"] <- "..." - # Split up compound names on , (swallowing spaces) duplicating their contents - individual_names <- strsplit(param_names, ",\\s*") - reps <- map_int(individual_names, length) - - setNames(rep.int(params, reps), unlist(individual_names)) + Map(list, + name = strsplit(param_names, ",\\s*"), + value = unlist(params) + ) } topic_params <- function(x) UseMethod("topic_params") @@ -392,26 +396,3 @@ get_rd_from_help <- function(package, alias) { internal_f("utils", ".getHelpFile")(help) } - - -# helpers ----------------------------------------------------------------- - -# Returns matching parameter name in haystack -match_param <- function(needle, haystack) { - if (needle %in% haystack) { - return(needle) - } - - if (substr(needle, 1, 1) == ".") { - if (needle %in% paste0(".", haystack)) { - return(substr(needle, 2, nchar(needle))) - } - } else { - if (paste0(".", needle) %in% haystack) { - return(paste0(".", needle)) - } - } - - NA -} - diff --git a/tests/testthat/test-rd-inherit.R b/tests/testthat/test-rd-inherit.R index 1421b02e..1836e935 100644 --- a/tests/testthat/test-rd-inherit.R +++ b/tests/testthat/test-rd-inherit.R @@ -325,17 +325,48 @@ test_that("multiple @inheritParam inherits from existing topics", { }) -test_that("@inheritParam can cope with multivariable argument definitions", { +test_that("@inheritParam can inherit multivariable arguments", { out <- roc_proc_text(rd_roclet(), " - #' My merge - #' - #' @inheritParams base::merge - mymerge <- function(x, y) {}")[[1]] - params <- out$get_value("param") - expect_equal(length(params), 2) - expect_equal(sort(names(params)), c("x", "y")) + #' A + #' @param x,y X and Y + A <- function(x, y) {} + + #' B + #' + #' @inheritParams A + B <- function(x, y) {}" + )[[2]] + expect_equal(out$get_value("param"), c("x,y" = "X and Y")) + + # Even when the names only match without . + out <- roc_proc_text(rd_roclet(), " + #' A + #' @param x,y X and Y + A <- function(x, y) {} + + #' B + #' + #' @inheritParams A + B <- function(.x, .y) {}" + )[[2]] + expect_equal(out$get_value("param"), c(".x,.y" = "X and Y")) +}) + +test_that("@inheritParam only inherits exact multiparam matches", { + out <- roc_proc_text(rd_roclet(), " + #' A + #' @param x,y X and Y + A <- function(x, y) {} + + #' B + #' + #' @inheritParams A + B <- function(x) {}" + )[[2]] + expect_equal(out$get_value("param"), NULL) }) + test_that("@inheritParam understands compound docs", { out <- roc_proc_text(rd_roclet(), " #' Title @@ -607,12 +638,3 @@ test_that("can find section in existing docs", { out <- find_sections(get_rd("base::attach")) expect_equal(out$title, "Good practice") }) - -# find_params ------------------------------------------------------------- - -test_that("find_params parses input", { - params <- find_params("utils::`?`", NULL) - expect_equal(names(params), c("topic", "type")) -}) - -