diff --git a/NEWS.md b/NEWS.md index e715ece4f..c7bb1d938 100644 --- a/NEWS.md +++ b/NEWS.md @@ -64,7 +64,7 @@ * `consecutive_mutate_linter()` for encouraging consecutive calls to `dplyr::mutate()` to be combined (part of #884, @MichaelChirico). * `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (part of #884, @MichaelChirico). * `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` (#2314 and 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` (#2313, #2314 and 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). * `object_overwrite_linter()` for discouraging re-use of upstream package exports as local variables (#2344, #2346 and part of #884, @MichaelChirico and @AshesITR). diff --git a/R/nrow_subset_linter.R b/R/nrow_subset_linter.R index f34246229..57ad645e1 100644 --- a/R/nrow_subset_linter.R +++ b/R/nrow_subset_linter.R @@ -14,6 +14,16 @@ #' linters = nrow_subset_linter() #' ) #' +#' lint( +#' text = "nrow(filter(x, is_treatment))", +#' linters = nrow_subset_linter() +#' ) +#' +#' lint( +#' text = "x %>% filter(x, is_treatment) %>% nrow()", +#' linters = nrow_subset_linter() +#' ) +#' #' # okay #' lint( #' text = "with(x, sum(is_treatment, na.rm = TRUE))", @@ -25,7 +35,7 @@ #' @include shared_constants.R #' @export nrow_subset_linter <- make_linter_from_function_xpath( - function_names = "subset", + function_names = c("subset", "filter"), xpath = glue(" parent::expr /parent::expr diff --git a/man/nrow_subset_linter.Rd b/man/nrow_subset_linter.Rd index 3e2edcb2a..a32627b79 100644 --- a/man/nrow_subset_linter.Rd +++ b/man/nrow_subset_linter.Rd @@ -21,6 +21,16 @@ lint( linters = nrow_subset_linter() ) +lint( + text = "nrow(filter(x, is_treatment))", + linters = nrow_subset_linter() +) + +lint( + text = "x \%>\% filter(x, is_treatment) \%>\% nrow()", + linters = nrow_subset_linter() +) + # okay lint( text = "with(x, sum(is_treatment, na.rm = TRUE))", diff --git a/tests/testthat/test-nrow_subset_linter.R b/tests/testthat/test-nrow_subset_linter.R index 2594f00ef..8f1d49f24 100644 --- a/tests/testthat/test-nrow_subset_linter.R +++ b/tests/testthat/test-nrow_subset_linter.R @@ -13,6 +13,14 @@ test_that("nrow_subset_linter blocks subset() cases", { ) }) +test_that("nrow_subset_linter blocks filter() cases", { + expect_lint( + "nrow(filter(x, y == z))", + rex::rex("Use arithmetic to count the number of rows satisfying a condition"), + nrow_subset_linter() + ) +}) + test_that("lints vectorize", { lint_msg <- rex::rex("Use arithmetic to count the number of rows satisfying a condition") @@ -21,10 +29,12 @@ test_that("lints vectorize", { nrow(subset(x, y == z)) subset(x) %>% transform(m = 2) nrow(subset(a, b == c)) + x %>% filter(y == z) %>% nrow() }"), list( list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 4L) + list(lint_msg, line_number = 4L), + list(lint_msg, line_number = 5L) ), nrow_subset_linter() ) @@ -35,7 +45,7 @@ test_that("linter is pipeline-aware", { lint_msg <- "Use arithmetic to count the number of rows satisfying a condition" expect_lint("x %>% subset(y == z) %>% nrow()", lint_msg, linter) - expect_lint("subset(x) %>% nrow()", lint_msg, linter) + expect_lint("filter(x, y == z) %>% nrow()", lint_msg, linter) skip_if_not_r_version("4.1.0") expect_lint("x |> subset(y == z) |> nrow()", lint_msg, linter)