From 8e32bee940e09fb881c8804b983cc183ab4a04ec Mon Sep 17 00:00:00 2001 From: Salim B Date: Wed, 6 Sep 2023 01:01:48 +0200 Subject: [PATCH 1/6] opt out of linting filter() in conjunct_test_linter fixes https://github.com/r-lib/lintr/issues/2108 --- R/conjunct_test_linter.R | 46 ++++++++++++++++++++++++++----------- man/conjunct_test_linter.Rd | 24 +++++++++++++++---- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/R/conjunct_test_linter.R b/R/conjunct_test_linter.R index 7d8461b2d..e3cb9e118 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,12 @@ #' 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 +50,17 @@ #' 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 +117,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..7ccb3a834 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,12 @@ 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 +60,12 @@ 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. From f5c599b19a9b03d6d6ed3e71f53dc95894b76069 Mon Sep 17 00:00:00 2001 From: Salim B Date: Wed, 6 Sep 2023 23:35:20 +0200 Subject: [PATCH 2/6] Use tidyverse style in examples --- R/conjunct_test_linter.R | 12 ++++++++---- man/conjunct_test_linter.Rd | 12 ++++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/R/conjunct_test_linter.R b/R/conjunct_test_linter.R index e3cb9e118..af2028fb6 100644 --- a/R/conjunct_test_linter.R +++ b/R/conjunct_test_linter.R @@ -34,8 +34,10 @@ #' ) #' #' lint( -#' text = "dplyr::filter(mtcars, -#' mpg > 20 & vs == 0)", +#' text = "dplyr::filter( +#' mtcars, +#' mpg > 20 & vs == 0 +#' )", #' linters = conjunct_test_linter() #' ) #' @@ -51,8 +53,10 @@ #' ) #' #' lint( -#' text = "dplyr::filter(mtcars, -#' mpg > 20 & vs == 0)", +#' text = "dplyr::filter( +#' mtcars, +#' mpg > 20 & vs == 0 +#' )", #' linters = conjunct_test_linter(allow_filter = TRUE) #' ) #' diff --git a/man/conjunct_test_linter.Rd b/man/conjunct_test_linter.Rd index 7ccb3a834..308e11b64 100644 --- a/man/conjunct_test_linter.Rd +++ b/man/conjunct_test_linter.Rd @@ -44,8 +44,10 @@ lint( ) lint( - text = "dplyr::filter(mtcars, - mpg > 20 & vs == 0)", + text = "dplyr::filter( + mtcars, + mpg > 20 & vs == 0 + )", linters = conjunct_test_linter() ) @@ -61,8 +63,10 @@ lint( ) lint( - text = "dplyr::filter(mtcars, - mpg > 20 & vs == 0)", + text = "dplyr::filter( + mtcars, + mpg > 20 & vs == 0 + )", linters = conjunct_test_linter(allow_filter = TRUE) ) From b37e4010690c82118d0610fdc0b2e6ec2c122b90 Mon Sep 17 00:00:00 2001 From: Salim B Date: Wed, 6 Sep 2023 23:35:54 +0200 Subject: [PATCH 3/6] Add test for conjunct_test_linter(allow_filter = TRUE) --- tests/testthat/test-conjunct_test_linter.R | 8 ++++++++ 1 file changed, 8 insertions(+) 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() From 99e3dd137dab86a0a4fdd93324e3dce670333370 Mon Sep 17 00:00:00 2001 From: Salim B Date: Wed, 6 Sep 2023 23:36:05 +0200 Subject: [PATCH 4/6] Add NEWS bullet --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 4aa15a272..d9e46a116 100644 --- a/NEWS.md +++ b/NEWS.md @@ -33,6 +33,7 @@ * `seq_linter()` recommends `rev()` in the lint message for lints like `nrow(x):1` (#1542, @MichaelChirico). * New `xp_call_name()` helper to facilitate writing custom linters (#2023, @MichaelChirico). This helper converts a matched XPath to the R function to which it corresponds. This is useful for including the "offending" function in the lint's message. * `function_argument_linter()` detects usage of `missing()` for the linted argument (#1546, @MichaelChirico). The simplest fix for `function_argument_linter()` lints is typically to set that argument to `NULL` by default, in which case it's usually preferable to update function logic checking `missing()` to check `is.null()` instead. +* Allow to opt out of linting `filter()` expressions in `conjunct_test_linter` via new `allow_filter` argument which defaults to `FALSE` (#2110, @salim-b) ### New linters From 3638b4adba954cadfdf9b3d6267d80fa620628f3 Mon Sep 17 00:00:00 2001 From: Salim B Date: Wed, 6 Sep 2023 23:46:12 +0200 Subject: [PATCH 5/6] Merge NEWS items --- NEWS.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index d9e46a116..f5e678d70 100644 --- a/NEWS.md +++ b/NEWS.md @@ -27,13 +27,12 @@ + `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). * New `xp_call_name()` helper to facilitate writing custom linters (#2023, @MichaelChirico). This helper converts a matched XPath to the R function to which it corresponds. This is useful for including the "offending" function in the lint's message. * `function_argument_linter()` detects usage of `missing()` for the linted argument (#1546, @MichaelChirico). The simplest fix for `function_argument_linter()` lints is typically to set that argument to `NULL` by default, in which case it's usually preferable to update function logic checking `missing()` to check `is.null()` instead. -* Allow to opt out of linting `filter()` expressions in `conjunct_test_linter` via new `allow_filter` argument which defaults to `FALSE` (#2110, @salim-b) ### New linters From 88734f31e1e7cc081916d999ce40a047e2c04937 Mon Sep 17 00:00:00 2001 From: Salim B Date: Wed, 6 Sep 2023 23:50:23 +0200 Subject: [PATCH 6/6] Write examples on fewer lines --- R/conjunct_test_linter.R | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/R/conjunct_test_linter.R b/R/conjunct_test_linter.R index af2028fb6..26489206c 100644 --- a/R/conjunct_test_linter.R +++ b/R/conjunct_test_linter.R @@ -34,10 +34,7 @@ #' ) #' #' lint( -#' text = "dplyr::filter( -#' mtcars, -#' mpg > 20 & vs == 0 -#' )", +#' text = "dplyr::filter(mtcars, mpg > 20 & vs == 0)", #' linters = conjunct_test_linter() #' ) #' @@ -53,10 +50,7 @@ #' ) #' #' lint( -#' text = "dplyr::filter( -#' mtcars, -#' mpg > 20 & vs == 0 -#' )", +#' text = "dplyr::filter(mtcars, mpg > 20 & vs == 0)", #' linters = conjunct_test_linter(allow_filter = TRUE) #' ) #'