diff --git a/NEWS.md b/NEWS.md index 52628d97d..426378ac1 100644 --- a/NEWS.md +++ b/NEWS.md @@ -123,6 +123,7 @@ function calls. (#850, #851, @renkun-ken) * `paste_linter()` lint for common mis-use of `paste()` and `paste()`: + `paste0()` encouraged instead of `paste(sep = "")` + `toString()` or `glue::glue_collapse()` encouraged instead of `paste(x, collapse = ", ")` + + `sep=` passed to `paste0()` -- typically a mistake (extension for #998) * `nested_ifelse_linter()` Prevent nested calls to `ifelse()` like `ifelse(A, x, ifelse(B, y, z))`, and similar * `condition_message_linter` Prevent condition messages from being constructed like `stop(paste(...))` (where just `stop(...)` is preferable) * `redundant_ifelse_linter()` Prevent usage like `ifelse(A & B, TRUE, FALSE)` or `ifelse(C, 0, 1)` (the latter is `as.numeric(!C)`) diff --git a/R/paste_linter.R b/R/paste_linter.R index 9d28dc358..cbd7a3f53 100644 --- a/R/paste_linter.R +++ b/R/paste_linter.R @@ -6,6 +6,8 @@ #' 1. Block usage of [paste()] with `sep = ""`. [paste0()] is a faster, more concise alternative. #' 2. Block usage of `paste()` or `paste0()` with `collapse = ", "`. [toString()] is a direct #' wrapper for this, and alternatives like [glue::glue_collapse()] might give better messages for humans. +#' 3. Block usage of `paste0()` that supplies `sep=` -- this is not a formal argument to `paste0`, and +#' is likely to be a mistake. #' #' @evalRd rd_tags("paste_linter") #' @param allow_empty_sep Logical, default `FALSE`. If `TRUE`, usage of @@ -69,6 +71,19 @@ paste_linter <- function(allow_empty_sep = FALSE, allow_to_string = FALSE) { ) } - c(empty_sep_lints, to_string_lints) + paste0_sep_xpath <- "//expr[ + expr[SYMBOL_FUNCTION_CALL[text() = 'paste0']] + and SYMBOL_SUB[text() = 'sep'] + ]" + paste0_sep_expr <- xml2::xml_find_all(xml, paste0_sep_xpath) + paste0_sep_lints <- lapply( + paste0_sep_expr, + xml_nodes_to_lint, + source_file = source_file, + lint_message = "sep= is not a formal argument to paste0(); did you mean to use paste(), or collapse=?", + type = "warning" + ) + + c(empty_sep_lints, to_string_lints, paste0_sep_lints) }) } diff --git a/man/paste_linter.Rd b/man/paste_linter.Rd index 724be6a6a..ed024ee0c 100644 --- a/man/paste_linter.Rd +++ b/man/paste_linter.Rd @@ -22,6 +22,8 @@ The following issues are linted by default by this linter \item Block usage of \code{\link[=paste]{paste()}} with \code{sep = ""}. \code{\link[=paste0]{paste0()}} is a faster, more concise alternative. \item Block usage of \code{paste()} or \code{paste0()} with \code{collapse = ", "}. \code{\link[=toString]{toString()}} is a direct wrapper for this, and alternatives like \code{\link[glue:glue_collapse]{glue::glue_collapse()}} might give better messages for humans. +\item Block usage of \code{paste0()} that supplies \verb{sep=} -- this is not a formal argument to \code{paste0}, and +is likely to be a mistake. } } \seealso{ diff --git a/tests/testthat/test-paste_linter.R b/tests/testthat/test-paste_linter.R index c078f1377..6026f514e 100644 --- a/tests/testthat/test-paste_linter.R +++ b/tests/testthat/test-paste_linter.R @@ -64,3 +64,11 @@ test_that("paste_linter respects non-default arguments", { expect_lint("paste(collapse = ', ', x)", NULL, paste_linter(allow_to_string = TRUE)) expect_lint("paste0(foo(x), collapse = ', ')", NULL, paste_linter(allow_to_string = TRUE)) }) + +test_that("paste_linter catches use of paste0 with sep=", { + expect_lint( + "paste0(x, y, sep = '')", + rex::rex("sep= is not a formal argument to paste0();"), + paste_linter() + ) +})