diff --git a/NEWS.md b/NEWS.md index a144aa7d9..e33d877cc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,12 +11,17 @@ + `with_defaults()`. + Linters `closed_curly_linter()`, `open_curly_linter()`, `paren_brace_linter()`, and `semicolon_terminator_linter()`. * Argument `interpret_glue` to `object_usage_linter()` is deprecated in favor of the more general `interpret_extensions`, in which `"glue"` is present by default (#1472, @MichaelChirico). See the description below. +* The default for `pipe_consistency_linter()` is changed from `"auto"` (require one pipe style, either magrittr or native) to `"|>"` (R native pipe required) to coincide with the same change in the Tidyverse Style Guide (#2707, @MichaelChirico). ## Bug fixes * `Lint()`, and thus all linters, ensures that the returned object's `message` attribute is consistently a simple character string (and not, for example, an object of class `"glue"`; #2740, @MichaelChirico). * Files with encoding inferred from settings read more robustly under `lint(parse_settings = TRUE)` (#2803, @MichaelChirico). +## Changes to default linters + +* `pipe_consistency_linter()`, with its new default to enforce the native pipe `|>`, is now a default linter, since it corresponds directly to a rule in the Tidyverse Style Guide (#2707, @MichaelChirico). + ## New and improved features * `brace_linter()`' has a new argument `function_bodies` (default `"multi_line"`) which controls when to require function bodies to be wrapped in curly braces, with the options `"always"`, `"multi_line"` (only require curly braces when a function body spans multiple lines), `"not_inline"` (only require curly braces when a function body starts on a new line) and `"never"` (#1807, #2240, @salim-b). diff --git a/R/pipe_consistency_linter.R b/R/pipe_consistency_linter.R index 89ee40952..81df66bb9 100644 --- a/R/pipe_consistency_linter.R +++ b/R/pipe_consistency_linter.R @@ -1,11 +1,11 @@ #' Pipe consistency linter #' -#' Check that pipe operators are used consistently by file, or optionally -#' specify one valid pipe operator. +#' Check that the recommended pipe operator is used, or more conservatively that +#' pipes are consistent by file. #' -#' @param pipe Which pipe operator is valid (either `"%>%"` or `"|>"`). By default -#' (`"auto"`), the linter has no preference but will check that each file uses -#' only one type of pipe operator. +#' @param pipe Which pipe operator is valid (either `"%>%"` or `"|>"`). The default +#' is the native pipe (`|>`). `"auto"` will instead +#' only enforce consistency, i.e., that in any given file there is only one pipe. #' #' @examples #' # will produce lints @@ -21,18 +21,20 @@ #' #' # okay #' lint( -#' text = "1:3 %>% mean() %>% as.character()", +#' text = "1:3 |> mean() |> as.character()", #' linters = pipe_consistency_linter() #' ) #' #' lint( -#' text = "1:3 |> mean() |> as.character()", -#' linters = pipe_consistency_linter() +#' text = "1:3 %>% mean() %>% as.character()", +#' linters = pipe_consistency_linter("%>%") #' ) #' @evalRd rd_tags("pipe_consistency_linter") -#' @seealso [linters] for a complete list of linters available in lintr. +#' @seealso +#' - [linters] for a complete list of linters available in lintr. +#' - #' @export -pipe_consistency_linter <- function(pipe = c("auto", "%>%", "|>")) { +pipe_consistency_linter <- function(pipe = c("|>", "%>%", "auto")) { pipe <- match.arg(pipe) xpath_magrittr <- glue("//SPECIAL[{ xp_text_in_table(magrittr_pipes) }]") @@ -64,10 +66,11 @@ pipe_consistency_linter <- function(pipe = c("auto", "%>%", "|>")) { type = "style" ) } else if (pipe == "|>" && n_magrittr > 0L) { + lint_message <- sprintf("Use the |> pipe operator instead of the %s pipe operator.", xml_text(match_magrittr)) xml_nodes_to_lints( xml = match_magrittr, source_expression = source_expression, - lint_message = "Use the |> pipe operator instead of the %>% pipe operator.", + lint_message = lint_message, type = "style" ) } else { diff --git a/R/zzz.R b/R/zzz.R index c2714ec8f..dc3f230da 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -26,6 +26,7 @@ default_linters <- modify_defaults( object_name_linter(), object_usage_linter(), paren_body_linter(), + pipe_consistency_linter(), pipe_continuation_linter(), quotes_linter(), return_linter(), diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 66e90379d..c2ad10c75 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -76,7 +76,7 @@ package_hooks_linter,style correctness package_development paren_body_linter,style readability default paste_linter,best_practices consistency configurable pipe_call_linter,style readability -pipe_consistency_linter,style readability configurable +pipe_consistency_linter,default style readability configurable pipe_continuation_linter,style readability default pipe_return_linter,best_practices common_mistakes print_linter,best_practices consistency diff --git a/man/default_linters.Rd b/man/default_linters.Rd index cdaa763c7..e226a1332 100644 --- a/man/default_linters.Rd +++ b/man/default_linters.Rd @@ -5,7 +5,7 @@ \alias{default_linters} \title{Default linters} \format{ -An object of class \code{list} of length 25. +An object of class \code{list} of length 26. } \usage{ default_linters @@ -38,6 +38,7 @@ The following linters are tagged with 'default': \item{\code{\link{object_name_linter}}} \item{\code{\link{object_usage_linter}}} \item{\code{\link{paren_body_linter}}} +\item{\code{\link{pipe_consistency_linter}}} \item{\code{\link{pipe_continuation_linter}}} \item{\code{\link{quotes_linter}}} \item{\code{\link{return_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index 5623666dd..88db1b4c7 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -22,7 +22,7 @@ The following tags exist: \item{\link[=configurable_linters]{configurable} (44 linters)} \item{\link[=consistency_linters]{consistency} (33 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} -\item{\link[=default_linters]{default} (25 linters)} +\item{\link[=default_linters]{default} (26 linters)} \item{\link[=efficiency_linters]{efficiency} (30 linters)} \item{\link[=executing_linters]{executing} (6 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} @@ -111,7 +111,7 @@ The following linters exist: \item{\code{\link{paren_body_linter}} (tags: default, readability, style)} \item{\code{\link{paste_linter}} (tags: best_practices, configurable, consistency)} \item{\code{\link{pipe_call_linter}} (tags: readability, style)} -\item{\code{\link{pipe_consistency_linter}} (tags: configurable, readability, style)} +\item{\code{\link{pipe_consistency_linter}} (tags: configurable, default, readability, style)} \item{\code{\link{pipe_continuation_linter}} (tags: default, readability, style)} \item{\code{\link{pipe_return_linter}} (tags: best_practices, common_mistakes)} \item{\code{\link{print_linter}} (tags: best_practices, consistency)} diff --git a/man/pipe_consistency_linter.Rd b/man/pipe_consistency_linter.Rd index ff53277a9..9cb188a79 100644 --- a/man/pipe_consistency_linter.Rd +++ b/man/pipe_consistency_linter.Rd @@ -4,16 +4,16 @@ \alias{pipe_consistency_linter} \title{Pipe consistency linter} \usage{ -pipe_consistency_linter(pipe = c("auto", "\%>\%", "|>")) +pipe_consistency_linter(pipe = c("|>", "\%>\%", "auto")) } \arguments{ -\item{pipe}{Which pipe operator is valid (either \code{"\%>\%"} or \code{"|>"}). By default -(\code{"auto"}), the linter has no preference but will check that each file uses -only one type of pipe operator.} +\item{pipe}{Which pipe operator is valid (either \code{"\%>\%"} or \code{"|>"}). The default +is the native pipe (\verb{|>}). \code{"auto"} will instead +only enforce consistency, i.e., that in any given file there is only one pipe.} } \description{ -Check that pipe operators are used consistently by file, or optionally -specify one valid pipe operator. +Check that the recommended pipe operator is used, or more conservatively that +pipes are consistent by file. } \examples{ # will produce lints @@ -29,18 +29,21 @@ lint( # okay lint( - text = "1:3 \%>\% mean() \%>\% as.character()", + text = "1:3 |> mean() |> as.character()", linters = pipe_consistency_linter() ) lint( - text = "1:3 |> mean() |> as.character()", - linters = pipe_consistency_linter() + text = "1:3 \%>\% mean() \%>\% as.character()", + linters = pipe_consistency_linter("\%>\%") ) } \seealso{ -\link{linters} for a complete list of linters available in lintr. +\itemize{ +\item \link{linters} for a complete list of linters available in lintr. +\item \url{https://style.tidyverse.org/pipes.html#magrittr} +} } \section{Tags}{ -\link[=configurable_linters]{configurable}, \link[=readability_linters]{readability}, \link[=style_linters]{style} +\link[=configurable_linters]{configurable}, \link[=default_linters]{default}, \link[=readability_linters]{readability}, \link[=style_linters]{style} } diff --git a/tests/testthat/default_linter_testcode.R b/tests/testthat/default_linter_testcode.R index 1cadc18cc..23c17c281 100644 --- a/tests/testthat/default_linter_testcode.R +++ b/tests/testthat/default_linter_testcode.R @@ -39,6 +39,7 @@ my_metric <- function(x) sum(x) + prod(x) # no_tab +# pipe_consistency # pipe_continuation # seq_linter # spaces_inside diff --git a/tests/testthat/test-pipe_consistency_linter.R b/tests/testthat/test-pipe_consistency_linter.R index 4a236b156..a7c5a24d3 100644 --- a/tests/testthat/test-pipe_consistency_linter.R +++ b/tests/testthat/test-pipe_consistency_linter.R @@ -2,42 +2,37 @@ test_that("pipe_consistency skips allowed usage", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter() - expect_lint("1:3 %>% mean() %>% as.character()", NULL, linter) - expect_lint("1:3 |> mean() |> as.character()", NULL, linter) + expect_no_lint("1:3 |> mean() |> as.character()", linter) # With no pipes - expect_lint("x <- 1:5", NULL, linter) + expect_no_lint("x <- 1:5", linter) # Across multiple lines - expect_lint( + expect_no_lint( trim_some(" - 1:3 %>% - mean() %>% + 1:3 |> + mean() |> as.character() "), - NULL, linter ) }) -test_that("pipe_consistency lints inconsistent usage", { +test_that("pipe_consistency lints blocked usage", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter() - expected_msg <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 1 instances of |>.") + lint_message <- rex::rex("Use the |> pipe operator instead of the %>% pipe operator.") + + + expect_lint("1:3 %>% mean() %>% as.character()", list(lint_message, lint_message), linter) expect_lint( "1:3 |> mean() %>% as.character()", - list( - list(message = expected_msg, line_number = 1L, column_number = 5L), - list(message = expected_msg, line_number = 1L, column_number = 15L) - ), + list(lint_message, line_number = 1L, column_number = 15L), linter ) expect_lint( "1:3 %>% mean() |> as.character()", - list( - list(message = expected_msg, line_number = 1L, column_number = 5L), - list(message = expected_msg, line_number = 1L, column_number = 16L) - ), + list(lint_message, line_number = 1L, column_number = 5L), linter ) @@ -47,21 +42,13 @@ test_that("pipe_consistency lints inconsistent usage", { mean() |> as.character() "), - list( - list(message = expected_msg, line_number = 1L, column_number = 5L), - list(message = expected_msg, line_number = 2L, column_number = 10L) - ), + list(lint_message, line_number = 1L, column_number = 5L), linter ) - expected_msg_multi <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 2 instances of |>.") expect_lint( "1:3 |> sort() |> mean() %>% as.character()", - list( - list(message = expected_msg_multi, line_number = 1L, column_number = 5L), - list(message = expected_msg_multi, line_number = 1L, column_number = 15L), - list(message = expected_msg_multi, line_number = 1L, column_number = 25L) - ), + list(lint_message, line_number = 1L, column_number = 25L), linter ) }) @@ -71,7 +58,8 @@ test_that("pipe_consistency_linter works with |> argument", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter(pipe = "|>") - expected_message <- rex::rex("Use the |> pipe operator instead of the %>% pipe operator.") + lint_message <- rex::rex("Use the |> pipe operator instead of the %>% pipe operator.") + lint_message_tee <- rex::rex("Use the |> pipe operator instead of the %T>% pipe operator.") expect_lint( trim_some(" @@ -80,8 +68,8 @@ test_that("pipe_consistency_linter works with |> argument", { as.character() "), list( - list(message = expected_message, line_number = 1L, column_number = 5L), - list(message = expected_message, line_number = 2L, column_number = 10L) + list(lint_message, line_number = 1L, column_number = 5L), + list(lint_message, line_number = 2L, column_number = 10L) ), linter ) @@ -92,13 +80,12 @@ test_that("pipe_consistency_linter works with |> argument", { mean() %>% as.character() "), - list(message = expected_message, line_number = 2L, column_number = 10L), + list(lint_message, line_number = 2L, column_number = 10L), linter ) - expect_lint( + expect_no_lint( "1:3 |> mean() |> as.character()", - NULL, linter ) @@ -108,7 +95,19 @@ test_that("pipe_consistency_linter works with |> argument", { mean() %>% as.character() "), - list(message = expected_message, line_number = 2L, column_number = 10L), + list(lint_message, line_number = 2L, column_number = 10L), + linter + ) + + expect_lint( + "1:3 %>% mean() %T>% print()", + list(lint_message, lint_message_tee), + linter + ) + + expect_lint( + "1:3 |> mean() %T>% print()", + list(lint_message_tee, line_number = 1L, column_number = 15L), linter ) }) @@ -134,9 +133,8 @@ test_that("pipe_consistency_linter works with %>% argument", { linter ) - expect_lint( + expect_no_lint( "1:3 %>% mean() %>% as.character()", - NULL, linter ) @@ -151,17 +149,62 @@ test_that("pipe_consistency_linter works with %>% argument", { ) }) -test_that("pipe_consistency_linter works with other magrittr pipes", { +test_that("simply enforcing a consistent style is supported", { skip_if_not_r_version("4.1.0") - linter <- pipe_consistency_linter() - expected_message <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 1 instances of |>.") - expect_lint("1:3 %>% mean() %T% print()", NULL, linter) + linter <- pipe_consistency_linter("auto") + lint_message <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 1 instances of |>.") + + expect_no_lint("1:3 %>% mean() %>% as.character()", linter) + + expect_lint( + "1:3 |> mean() %>% as.character()", + list( + list(lint_message, line_number = 1L, column_number = 5L), + list(lint_message, line_number = 1L, column_number = 15L) + ), + linter + ) + + expect_lint( + "1:3 %>% mean() |> as.character()", + list( + list(lint_message, line_number = 1L, column_number = 5L), + list(lint_message, line_number = 1L, column_number = 16L) + ), + linter + ) + + expect_lint( + trim_some(" + 1:3 %>% + mean() |> + as.character() + "), + list( + list(lint_message, line_number = 1L, column_number = 5L), + list(lint_message, line_number = 2L, column_number = 10L) + ), + linter + ) + + lint_message_multi <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 2 instances of |>.") + expect_lint( + "1:3 |> sort() |> mean() %>% as.character()", + list( + list(lint_message_multi, line_number = 1L, column_number = 5L), + list(lint_message_multi, line_number = 1L, column_number = 15L), + list(lint_message_multi, line_number = 1L, column_number = 25L) + ), + linter + ) + + expect_no_lint("1:3 %>% mean() %T% print()", linter) expect_lint( "1:3 |> mean() %T>% print()", list( - list(message = expected_message, line_number = 1L, column_number = 5L), - list(message = expected_message, line_number = 1L, column_number = 15L) + list(lint_message, line_number = 1L, column_number = 5L), + list(lint_message, line_number = 1L, column_number = 15L) ), linter )