extend conjunct_test_linter for dplyr::filter()

NEWS.md

@@ -23,6 +23,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).
 * `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).
 

R/conjunct_test_linter.R

@@ -1,4 +1,4 @@
-#' Force `&&` conditions in `expect_true()` and `expect_false()` to be written separately
+#' Force `&&` conditions to be written separately where appropriate
 #'
 #' For readability of test outputs, testing only one thing per call to
 #'   [testthat::expect_true()] is preferable, i.e.,
@@ -7,6 +7,11 @@
 #'
 #' 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.
+#'
 #' @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.
 #'
@@ -44,26 +49,37 @@
 conjunct_test_linter <- function(allow_named_stopifnot = TRUE) {
   expect_true_assert_that_xpath <- "
   //SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'assert_that']
-  /parent::expr
-  /following-sibling::expr[1][AND2]
+    /parent::expr
+    /following-sibling::expr[1][AND2]
+    /parent::expr
   "
   named_stopifnot_condition <- if (allow_named_stopifnot) "and not(preceding-sibling::*[1][self::EQ_SUB])" else ""
   stopifnot_xpath <- glue("
   //SYMBOL_FUNCTION_CALL[text() = 'stopifnot']
-  /parent::expr
-  /following-sibling::expr[1][AND2 {named_stopifnot_condition}]
+    /parent::expr
+    /following-sibling::expr[1][AND2 {named_stopifnot_condition}]
+    /parent::expr
   ")
   expect_false_xpath <- "
   //SYMBOL_FUNCTION_CALL[text() = 'expect_false']
-  /parent::expr
-  /following-sibling::expr[1][OR2]
+    /parent::expr
+    /following-sibling::expr[1][OR2]
+    /parent::expr
   "
-  xpath <- paste0(
-    c(expect_true_assert_that_xpath, stopifnot_xpath, expect_false_xpath),
-    "/parent::expr",
-    collapse = " | "
+  test_xpath <- paste(
+    expect_true_assert_that_xpath,
+    stopifnot_xpath,
+    expect_false_xpath,
+    sep = " | "
   )
 
+  filter_xpath <- "
+  //SYMBOL_FUNCTION_CALL[text() = 'filter']
+    /parent::expr[not(SYMBOL_PACKAGE[text() != 'dplyr'])]
+    /parent::expr
+    /expr[AND]
+  "
+
   Linter(function(source_expression) {
     # need the full file to also catch usages at the top level
     if (!is_lint_level(source_expression, "file")) {
@@ -72,24 +88,37 @@ conjunct_test_linter <- function(allow_named_stopifnot = TRUE) {
 
     xml <- source_expression$full_xml_parsed_content
 
-    bad_expr <- xml_find_all(xml, xpath)
-
-    if (length(bad_expr) == 0L) {
-      return(list())
-    }
+    test_expr <- xml_find_all(xml, test_xpath)
 
-    matched_fun <- xp_call_name(bad_expr)
-    operator <- xml_find_chr(bad_expr, "string(expr/*[self::AND2 or self::OR2])")
+    matched_fun <- xp_call_name(test_expr)
+    operator <- xml_find_chr(test_expr, "string(expr/*[self::AND2 or self::OR2])")
     replacement_fmt <- ifelse(
       matched_fun %in% c("expect_true", "expect_false"),
       "write multiple expectations like %1$s(A) and %1$s(B)",
       "write multiple conditions like %s(A, B)."
     )
     lint_message <- paste(
       sprintf("Instead of %s(A %s B),", matched_fun, operator),
-      sprintf(replacement_fmt, matched_fun),
+      # as.character() needed for 0-lint case where ifelse(logical(0)) returns logical(0)
+      sprintf(as.character(replacement_fmt), matched_fun),
       "The latter will produce better error messages in the case of failure."
     )
-    xml_nodes_to_lints(bad_expr, source_expression, lint_message = lint_message, type = "warning")
+    test_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)
+
+    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"
+    )
+
+    c(test_lints, filter_lints)
   })
 }

tests/testthat/test-conjunct_test_linter.R

@@ -109,3 +109,36 @@ test_that("conjunct_test_linter's allow_named_stopifnot argument works", {
     conjunct_test_linter(allow_named_stopifnot = FALSE)
   )
 })
+
+test_that("conjunct_test_linter skips allowed usages", {
+  linter <- conjunct_test_linter()
+
+  expect_lint("dplyr::filter(DF, A, B)", NULL, linter)
+  expect_lint("dplyr::filter(DF, !(A & B))", NULL, linter)
+  # | is the "top-level" operator here
+  expect_lint("dplyr::filter(DF, A & B | C)", NULL, linter)
+  expect_lint("dplyr::filter(DF, A | B & C)", NULL, linter)
+})
+
+test_that("conjunct_test_linter blocks simple disallowed usages", {
+  linter <- conjunct_test_linter()
+  lint_msg <- rex::rex("Use dplyr::filter(DF, A, B) instead of dplyr::filter(DF, A & B)")
+
+  expect_lint("dplyr::filter(DF, A & B)", lint_msg, linter)
+  expect_lint("dplyr::filter(DF, A & B & C)", lint_msg, linter)
+
+  # more common usage, in pipes
+  expect_lint("DF %>% dplyr::filter(A & B)", lint_msg, linter)
+})
+
+test_that("filter() is assumed to be dplyr::filter() by default, unless o/w specified", {
+  linter <- conjunct_test_linter()
+
+  expect_lint("stats::filter(A & B)", NULL, linter)
+  expect_lint("ns::filter(A & B)", NULL, linter)
+  expect_lint(
+    "DF %>% filter(A & B)",
+    rex::rex("Use dplyr::filter(DF, A, B) instead of dplyr::filter(DF, A & B)"),
+    linter
+  )
+})