diff --git a/NEWS.md b/NEWS.md index 4aa15a272..f5e678d70 100644 --- a/NEWS.md +++ 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). diff --git a/R/conjunct_test_linter.R b/R/conjunct_test_linter.R index 7d8461b2d..26489206c 100644 --- a/R/conjunct_test_linter.R +++ b/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 }) } diff --git a/man/conjunct_test_linter.Rd b/man/conjunct_test_linter.Rd index 08a68b748..308e11b64 100644 --- a/man/conjunct_test_linter.Rd +++ b/man/conjunct_test_linter.Rd @@ -4,11 +4,13 @@ \alias{conjunct_test_linter} \title{Force \code{&&} conditions to be written separately where appropriate} \usage{ -conjunct_test_linter(allow_named_stopifnot = TRUE) +conjunct_test_linter(allow_named_stopifnot = TRUE, allow_filter = FALSE) } \arguments{ \item{allow_named_stopifnot}{Logical, \code{TRUE} by default. If \code{FALSE}, "named" calls to \code{stopifnot()}, available since R 4.0.0 to provide helpful messages for test failures, are also linted.} + +\item{allow_filter}{Logical, \code{FALSE} by default. If \code{TRUE}, \code{filter()} expressions are not linted.} } \description{ For readability of test outputs, testing only one thing per call to @@ -19,10 +21,10 @@ For readability of test outputs, testing only one thing per call to \details{ Similar reasoning applies to \code{&&} usage inside \code{\link[=stopifnot]{stopifnot()}} and \code{assertthat::assert_that()} calls. -Relatedly, \code{dplyr::filter(DF, A & B)} is the same as \code{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 \code{filter()} are \code{dplyr::filter()}; if you're using another function named \code{filter()}, -e.g. \code{\link[stats:filter]{stats::filter()}}, please namespace-qualify it to avoid false positives. +Relatedly, \code{dplyr::filter(DF, A & B)} is the same as \code{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 \code{filter()} are \code{dplyr::filter()}; +if you're using another function named \code{filter()}, e.g. \code{\link[stats:filter]{stats::filter()}}, please namespace-qualify it to avoid +false positives. You can omit linting \code{filter()} expressions altogether via \code{allow_filter = TRUE}. } \examples{ # will produce lints @@ -41,6 +43,14 @@ lint( 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))", @@ -52,6 +62,14 @@ lint( linters = conjunct_test_linter(allow_named_stopifnot = TRUE) ) +lint( + text = "dplyr::filter( + mtcars, + mpg > 20 & vs == 0 + )", + linters = conjunct_test_linter(allow_filter = TRUE) +) + } \seealso{ \link{linters} for a complete list of linters available in lintr. diff --git a/tests/testthat/test-conjunct_test_linter.R b/tests/testthat/test-conjunct_test_linter.R index d7dfbf7f5..4d0be1f89 100644 --- a/tests/testthat/test-conjunct_test_linter.R +++ b/tests/testthat/test-conjunct_test_linter.R @@ -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()