Allow to opt out of linting filter() in conjunct_test_linter (#2110)

* opt out of linting filter() in conjunct_test_linter

fixes #2108

* Use tidyverse style in examples

* Add test for conjunct_test_linter(allow_filter = TRUE)

* Add NEWS bullet

* Merge NEWS items

* Write examples on fewer lines

Author: salim-b

NEWS.md

@@ -27,7 +27,7 @@
   + `yoda_test_linter()`
 * `sprintf_linter()` is pipe-aware, so that `x %>% sprintf(fmt = "%s")` no longer lints (#1943, @MichaelChirico).
 * `line_length_linter()` helpfully includes the line length in the lint message (#2057, @MichaelChirico).
-* `conjunct_test_linter()` also lints usage like `dplyr::filter(x, A & B)` in favor of using `dplyr::filter(x, A, B)` (part of #884, @MichaelChirico).
+* `conjunct_test_linter()` also lints usage like `dplyr::filter(x, A & B)` in favor of using `dplyr::filter(x, A, B)` unless `allow_filter = TRUE` (part of #884, @MichaelChirico; #2110, @salim-b).
 * `sort_linter()` checks for code like `x == sort(x)` which is better served by using the function `is.unsorted()` (part of #884, @MichaelChirico).
 * `paste_linter()` gains detection for file paths that are better constructed with `file.path()`, e.g. `paste0(dir, "/", file)` would be better as `file.path(dir, file)` (part of #884, @MichaelChirico).
 * `seq_linter()` recommends `rev()` in the lint message for lints like `nrow(x):1` (#1542, @MichaelChirico).

R/conjunct_test_linter.R

@@ -7,13 +7,14 @@
 #'
 #' Similar reasoning applies to `&&` usage inside [stopifnot()] and `assertthat::assert_that()` calls.
 #'
-#' Relatedly, `dplyr::filter(DF, A & B)` is the same as `dplyr::filter(DF, A, B)`, but the
-#'   latter will be more readable / easier to format for long conditions. Note that this linter
-#'   assumes usages of `filter()` are `dplyr::filter()`; if you're using another function named `filter()`,
-#'   e.g. [stats::filter()], please namespace-qualify it to avoid false positives.
+#' Relatedly, `dplyr::filter(DF, A & B)` is the same as `dplyr::filter(DF, A, B)`, but the latter will be more readable
+#'   / easier to format for long conditions. Note that this linter assumes usages of `filter()` are `dplyr::filter()`;
+#'   if you're using another function named `filter()`, e.g. [stats::filter()], please namespace-qualify it to avoid
+#'   false positives. You can omit linting `filter()` expressions altogether via `allow_filter = TRUE`.
 #'
 #' @param allow_named_stopifnot Logical, `TRUE` by default. If `FALSE`, "named" calls to `stopifnot()`,
 #'   available since R 4.0.0 to provide helpful messages for test failures, are also linted.
+#' @param allow_filter Logical, `FALSE` by default. If `TRUE`, `filter()` expressions are not linted.
 #'
 #' @examples
 #' # will produce lints
@@ -32,6 +33,11 @@
 #'   linters = conjunct_test_linter(allow_named_stopifnot = FALSE)
 #' )
 #'
+#' lint(
+#'   text = "dplyr::filter(mtcars, mpg > 20 & vs == 0)",
+#'   linters = conjunct_test_linter()
+#' )
+#'
 #' # okay
 #' lint(
 #'   text = "expect_true(x || (y && z))",
@@ -43,10 +49,16 @@
 #'   linters = conjunct_test_linter(allow_named_stopifnot = TRUE)
 #' )
 #'
+#' lint(
+#'   text = "dplyr::filter(mtcars, mpg > 20 & vs == 0)",
+#'   linters = conjunct_test_linter(allow_filter = TRUE)
+#' )
+#'
 #' @evalRd rd_tags("conjunct_test_linter")
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
-conjunct_test_linter <- function(allow_named_stopifnot = TRUE) {
+conjunct_test_linter <- function(allow_named_stopifnot = TRUE,
+                                 allow_filter = FALSE) {
   expect_true_assert_that_xpath <- "
   //SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'assert_that']
     /parent::expr
@@ -103,22 +115,26 @@ conjunct_test_linter <- function(allow_named_stopifnot = TRUE) {
       sprintf(as.character(replacement_fmt), matched_fun),
       "The latter will produce better error messages in the case of failure."
     )
-    test_lints <- xml_nodes_to_lints(
+    lints <- xml_nodes_to_lints(
       test_expr,
       source_expression = source_expression,
       lint_message = lint_message,
       type = "warning"
     )
 
-    filter_expr <- xml_find_all(xml, filter_xpath)
+    if (!allow_filter) {
+      filter_expr <- xml_find_all(xml, filter_xpath)
 
-    filter_lints <- xml_nodes_to_lints(
-      filter_expr,
-      source_expression = source_expression,
-      lint_message = "Use dplyr::filter(DF, A, B) instead of dplyr::filter(DF, A & B).",
-      type = "warning"
-    )
+      filter_lints <- xml_nodes_to_lints(
+        filter_expr,
+        source_expression = source_expression,
+        lint_message = "Use dplyr::filter(DF, A, B) instead of dplyr::filter(DF, A & B).",
+        type = "warning"
+      )
+
+      lints <- c(lints, filter_lints)
+    }
 
-    c(test_lints, filter_lints)
+    lints
   })
 }

man/conjunct_test_linter.Rd

@@ -131,6 +131,14 @@ test_that("conjunct_test_linter blocks simple disallowed usages", {
   expect_lint("DF %>% dplyr::filter(A & B)", lint_msg, linter)
 })
 
+test_that("conjunct_test_linter respects its allow_filter argument", {
+  linter <- conjunct_test_linter(allow_filter = TRUE)
+
+  expect_lint("dplyr::filter(DF, A & B)", NULL, linter)
+  expect_lint("dplyr::filter(DF, A & B & C)", NULL, linter)
+  expect_lint("DF %>% dplyr::filter(A & B)", NULL, linter)
+})
+
 test_that("filter() is assumed to be dplyr::filter() by default, unless o/w specified", {
   linter <- conjunct_test_linter()