From 5da20e0b8617a39cd5d317a7987023ffd7089d16 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 25 Mar 2022 13:44:46 -0700 Subject: [PATCH] New paste_sep_linter (#997) --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/paste_sep_linter.R | 33 ++++++++++++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/consistency_linters.Rd | 1 + man/linters.Rd | 5 ++-- man/paste_sep_linter.Rd | 17 +++++++++++++ tests/testthat/test-paste_sep_linter.R | 24 +++++++++++++++++++ 10 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 R/paste_sep_linter.R create mode 100644 man/paste_sep_linter.Rd create mode 100644 tests/testthat/test-paste_sep_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 085e4786d..f3b4dd29b 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -102,6 +102,7 @@ Collate: 'package_hooks_linter.R' 'paren_body_linter.R' 'paren_brace_linter.R' + 'paste_sep_linter.R' 'path_linters.R' 'pipe_call_linter.R' 'pipe_continuation_linter.R' diff --git a/NAMESPACE b/NAMESPACE index b12a56a94..45db3b27a 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -68,6 +68,7 @@ export(outer_negation_linter) export(package_hooks_linter) export(paren_body_linter) export(paren_brace_linter) +export(paste_sep_linter) export(pipe_call_linter) export(pipe_continuation_linter) export(semicolon_terminator_linter) diff --git a/NEWS.md b/NEWS.md index e520a8718..334c2b6d6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -102,6 +102,7 @@ function calls. (#850, #851, @renkun-ken) * `any_is_na_linter()` Require usage of `anyNA(x)` over `any(is.na(x))` * `outer_negation_linter()` Require usage of `!any(x)` over `all(!x)` and `!all(x)` over `any(!x)` * `numeric_leading_zero_linter()` Require a leading `0` in fractional numeric constants, e.g. `0.1` instead of `.1` + * `paste_sep_linter()` Require usage of `paste0()` over `paste(sep = "")` * `nested_ifelse_linter()` Prevent nested calls to `ifelse()` like `ifelse(A, x, ifelse(B, y, z))`, and similar * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico) * `infix_spaces_linter()` gains argument `exclude_operators` to disable lints on selected infix operators. By default, all "low-precedence" operators throw lints; see `?infix_spaces_linter` for an enumeration of these. (#914 @michaelchirico) diff --git a/R/paste_sep_linter.R b/R/paste_sep_linter.R new file mode 100644 index 000000000..a4150eb9c --- /dev/null +++ b/R/paste_sep_linter.R @@ -0,0 +1,33 @@ +#' Block usage of paste() with sep="" +#' +#' [paste0()] is a faster, more concise alternative to using `paste(sep = "")`. +#' +#' @evalRd rd_tags("paste_sep_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +paste_sep_linter <- function() { + Linter(function(source_file) { + if (length(source_file$xml_parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$xml_parsed_content + + # NB: string-length()=2 means '' or "" (*not* 'ab' or 'cd' which have length 4) + # TODO: adapt this for R>4.0 raw strings + xpath <- "//expr[ + expr[SYMBOL_FUNCTION_CALL[text() = 'paste']] + and SYMBOL_SUB[text() = 'sep']/following-sibling::expr[1][STR_CONST[string-length(text()) = 2]] + ]" + + bad_expr <- xml2::xml_find_all(xml, xpath) + + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file = source_file, + lint_message = 'paste0(...) is better than paste(..., sep = "").', + type = "warning" + )) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 01aac11d3..494977ad4 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -41,6 +41,7 @@ outer_negation_linter,readability efficiency best_practices package_hooks_linter,style correctness package_development paren_body_linter,style readability default paren_brace_linter,style readability default +paste_sep_linter,best_practices consistency pipe_call_linter,style readability pipe_continuation_linter,style readability default semicolon_terminator_linter,style readability default configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 367878163..d1b617485 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -30,6 +30,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{implicit_integer_linter}}} \item{\code{\link{nonportable_path_linter}}} \item{\code{\link{outer_negation_linter}}} +\item{\code{\link{paste_sep_linter}}} \item{\code{\link{seq_linter}}} \item{\code{\link{T_and_F_symbol_linter}}} \item{\code{\link{undesirable_function_linter}}} diff --git a/man/consistency_linters.Rd b/man/consistency_linters.Rd index 442765c81..1b3e2ebf6 100644 --- a/man/consistency_linters.Rd +++ b/man/consistency_linters.Rd @@ -18,6 +18,7 @@ The following linters are tagged with 'consistency': \item{\code{\link{no_tab_linter}}} \item{\code{\link{numeric_leading_zero_linter}}} \item{\code{\link{object_name_linter}}} +\item{\code{\link{paste_sep_linter}}} \item{\code{\link{seq_linter}}} \item{\code{\link{single_quotes_linter}}} \item{\code{\link{T_and_F_symbol_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 53e2792d3..a25ac0cfa 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,10 +17,10 @@ Documentation for linters is structured into tags to allow for easier discovery. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (23 linters)} +\item{\link[=best_practices_linters]{best_practices} (24 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} -\item{\link[=consistency_linters]{consistency} (8 linters)} +\item{\link[=consistency_linters]{consistency} (9 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (27 linters)} \item{\link[=efficiency_linters]{efficiency} (8 linters)} @@ -75,6 +75,7 @@ The following linters exist: \item{\code{\link{package_hooks_linter}} (tags: correctness, package_development, style)} \item{\code{\link{paren_body_linter}} (tags: default, readability, style)} \item{\code{\link{paren_brace_linter}} (tags: default, readability, style)} +\item{\code{\link{paste_sep_linter}} (tags: best_practices, consistency)} \item{\code{\link{pipe_call_linter}} (tags: readability, style)} \item{\code{\link{pipe_continuation_linter}} (tags: default, readability, style)} \item{\code{\link{semicolon_terminator_linter}} (tags: configurable, default, readability, style)} diff --git a/man/paste_sep_linter.Rd b/man/paste_sep_linter.Rd new file mode 100644 index 000000000..82f8f08b8 --- /dev/null +++ b/man/paste_sep_linter.Rd @@ -0,0 +1,17 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/paste_sep_linter.R +\name{paste_sep_linter} +\alias{paste_sep_linter} +\title{Block usage of paste() with sep=""} +\usage{ +paste_sep_linter() +} +\description{ +\code{\link[=paste0]{paste0()}} is a faster, more concise alternative to using \code{paste(sep = "")}. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency} +} diff --git a/tests/testthat/test-paste_sep_linter.R b/tests/testthat/test-paste_sep_linter.R new file mode 100644 index 000000000..2503b6f2a --- /dev/null +++ b/tests/testthat/test-paste_sep_linter.R @@ -0,0 +1,24 @@ +test_that("paste_sep_linter skips allowed usages", { + expect_lint("paste('a', 'b', 'c')", NULL, paste_sep_linter()) + expect_lint("paste('a', 'b', 'c', sep = ',')", NULL, paste_sep_linter()) + expect_lint("paste('a', 'b', collapse = '')", NULL, paste_sep_linter()) + expect_lint("cat(paste('a', 'b'), sep = '')", NULL, paste_sep_linter()) + expect_lint("sep <- ''; paste('a', sep)", NULL, paste_sep_linter()) + expect_lint("paste(sep = ',', '', 'a')", NULL, paste_sep_linter()) + + expect_lint("paste0('a', 'b', 'c')", NULL, paste_sep_linter()) +}) + +test_that("paste_sep_linter blocks simple disallowed usages", { + expect_lint( + "paste(sep = '', 'a', 'b')", + rex::rex('paste0(...) is better than paste(..., sep = "").'), + paste_sep_linter() + ) + + expect_lint( + "paste('a', 'b', sep = '')", + rex::rex('paste0(...) is better than paste(..., sep = "").'), + paste_sep_linter() + ) +})