Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend paste_linter to catch usages like paste0(sep="") #1074

Merged
merged 2 commits into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)`)
Expand Down
17 changes: 16 additions & 1 deletion R/paste_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
}
2 changes: 2 additions & 0 deletions man/paste_linter.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions tests/testthat/test-paste_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -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()
)
})