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

Enforce the native pipe by default #2796

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@
+ `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).

## 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).
Expand Down
25 changes: 14 additions & 11 deletions R/pipe_consistency_linter.R
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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.
#' - <https://style.tidyverse.org/pipes.html#magrittr>
#' @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) }]")
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions R/zzz.R
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion inst/lintr/linters.csv
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,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
Expand Down
3 changes: 2 additions & 1 deletion man/default_linters.Rd

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

4 changes: 2 additions & 2 deletions man/linters.Rd

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

25 changes: 14 additions & 11 deletions man/pipe_consistency_linter.Rd

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

1 change: 1 addition & 0 deletions tests/testthat/default_linter_testcode.R
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ my_metric <- function(x)
sum(x) + prod(x)

# no_tab
# pipe_consistency
# pipe_continuation
# seq_linter
# spaces_inside
Expand Down
127 changes: 85 additions & 42 deletions tests/testthat/test-pipe_consistency_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand All @@ -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
)
})
Expand All @@ -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("
Expand All @@ -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
)
Expand All @@ -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
)

Expand All @@ -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
)
})
Expand All @@ -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
)

Expand All @@ -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
)
Expand Down
Loading