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.

Sign up for GitHub

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

exclude %<>% from one_call_pipe_linter #2331

Merged
merged 2 commits into from
Nov 21, 2023
Merged

exclude %<>% from one_call_pipe_linter #2331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1abf1b1
exclude %<>%
MichaelChirico Nov 21, 2023
80b1436
document
MichaelChirico Nov 21, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
* `nested_pipe_linter()` for discouraging pipes within pipes, e.g. `df1 %>% inner_join(df2 %>% select(a, b))` (part of #884, @MichaelChirico).
* `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (part of #884, @MichaelChirico).
* `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico).
* `one_call_pipe_linter()` for discouraging one-step pipelines like `x |> as.character()` (#2330 and part of #884, @MichaelChirico).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we miss the bullet for the first PR? 😳

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes 🥲


### Lint accuracy fixes: removing false positives

Expand Down
9 changes: 8 additions & 1 deletion R/one_call_pipe_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,20 @@
#' linters = one_call_pipe_linter()
#' )
#'
#' # assignment pipe is exempted
#' lint(
#' text = "DF %<>% mutate(a = 2)",
#' linters = one_call_pipe_linter()
#' )
#'
#' @evalRd rd_tags("one_call_pipe_linter")
#' @seealso
#' - [linters] for a complete list of linters available in lintr.
#' - <https://style.tidyverse.org/pipes.html#short-pipes>
#' @export
one_call_pipe_linter <- function() {
pipes_cond <- xp_text_in_table(magrittr_pipes)
# exception for assignment pipe per #2330
pipes_cond <- xp_text_in_table(setdiff(magrittr_pipes, "%<>%"))

# preceding-sibling::SPECIAL: if there are ever two pipes, don't lint
# OP-LEFT-BRACKET/LBB: accept DT[...] %>% .[...] as a two-call pipe,
Expand Down
6 changes: 6 additions & 0 deletions man/one_call_pipe_linter.Rd
View file
Open in desktop

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

20 changes: 8 additions & 12 deletions tests/testthat/test-one_call_pipe_linter.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ test_that("one_call_pipe_linter skips allowed usages", {
expect_lint("foo(x) %>% bar()", NULL, linter)
# both calls in second step --> OK
expect_lint("x %>% foo(bar(.))", NULL, linter)

# assignment pipe is exempted
expect_lint("x %<>% as.character()", NULL, linter)
})

test_that("one_call_pipe_linter blocks simple disallowed usages", {
Expand Down Expand Up @@ -41,20 +44,13 @@ test_that("one_call_pipe_linter skips data.table chains", {

test_that("one_call_pipe_linter treats all pipes equally", {
linter <- one_call_pipe_linter()
lint_msg_part <- "Expressions with only a single call shouldn't use pipe "

expect_lint("foo %>% bar() %$% col", NULL, linter)
expect_lint("x %T>% foo()", rex::rex(lint_msg_part, "%T>%."), linter)
expect_lint("x %$%\n foo", rex::rex(lint_msg_part, "%$%."), linter)
expect_lint(
"x %T>% foo()",
rex::rex("Expressions with only a single call shouldn't use pipe %T>%."),
linter
)
expect_lint(
"x %$%\n foo()",
rex::rex("Expressions with only a single call shouldn't use pipe %$%."),
linter
)
expect_lint(
'data %>% filter(type == "console") %$% obscured_gaia_id %>% unique()',
'data %>% filter(type == "console") %$% obscured_id %>% unique()',
NULL,
linter
)
Expand All @@ -64,7 +60,7 @@ test_that("multiple lints are generated correctly", {
expect_lint(
trim_some("{
a %>% b()
c %$% d()
c %$% d
e %T>%
f()
}"),
Expand Down
Loading