From 66f0ed6bdd1b58e75343bd2e5b22dbff199038eb Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 16 Mar 2022 07:28:42 +0000 Subject: [PATCH 01/12] New expect_true_false_and_condition_linter --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 3 +- R/expect_true_false_and_condition_linter.R | 41 +++++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/expect_not_linter.Rd | 4 +- man/expect_true_false_and_condition_linter.Rd | 19 +++++++++ man/linters.Rd | 8 ++-- man/package_development_linters.Rd | 1 + man/readability_linters.Rd | 1 + ...t-expect_true_false_and_condition_linter.R | 24 +++++++++++ 12 files changed, 99 insertions(+), 6 deletions(-) create mode 100644 R/expect_true_false_and_condition_linter.R create mode 100644 man/expect_true_false_and_condition_linter.Rd create mode 100644 tests/testthat/test-expect_true_false_and_condition_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 3833a2482..8161c4b9d 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -67,6 +67,7 @@ Collate: 'expect_not_linter.R' 'expect_null_linter.R' 'expect_s3_class_linter.R' + 'expect_true_false_and_condition_linter.R' 'expect_type_linter.R' 'extract.R' 'extraction_operator_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 3f1fbd1db..110fdded0 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -35,6 +35,7 @@ export(expect_not_linter) export(expect_null_linter) export(expect_s3_class_linter) export(expect_s4_class_linter) +export(expect_true_false_and_condition_linter) export(expect_type_linter) export(extraction_operator_linter) export(function_left_parentheses_linter) diff --git a/NEWS.md b/NEWS.md index ad2291b76..67bc1914f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -91,7 +91,8 @@ function calls. (#850, #851, @renkun-ken) + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar + `expect_s3_class_linter()` Require usage of `expect_s3_class(x, k)` over `expect_equal(class(x), k)` and similar + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` - + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. + + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_ + + `expect_true_false_and_condition_linter()` Require multiple `expect_true()` tests over `expect_true(x && y)` and similar * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico) # lintr 2.0.1 diff --git a/R/expect_true_false_and_condition_linter.R b/R/expect_true_false_and_condition_linter.R new file mode 100644 index 000000000..f6ef507cf --- /dev/null +++ b/R/expect_true_false_and_condition_linter.R @@ -0,0 +1,41 @@ +#' Force && conditions in expect_true(), expect_false() to be written separately +#' +#' For readability of test outputs, testing only one thing per call to +#' [testthat::expect_true()] is preferable, i.e., +#' `expect_true(A); expect_true(B)` is better than `expect_true(A && B)`. +#' +#' @evalRd rd_tags("expect_true_false_and_condition_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +expect_true_false_and_condition_linter <- function() { + Linter(function(source_file) { + # need the full file to also catch usages at the top level + if (length(source_file$full_parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$full_xml_parsed_content + + xpath <- " + //expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'expect_false']] + /following-sibling::expr + /AND2 + " + + bad_expr <- xml2::xml_find_all(xml, xpath) + + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file = source_file, + message = paste( + "Write multiple && conditions in expect_true()/expect_false() as", + "individual unit tests, e.g. expect_true(x && y) becomes", + "expect_true(x) and expect_true(y). The latter will produce", + "better error messages in the case of failure." + ), + type = "warning", + global = TRUE + )) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 6a11db3de..5e39251f6 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -13,6 +13,7 @@ expect_not_linter,package_development best_practices readability expect_null_linter,package_development best_practices expect_s3_class_linter,package_development best_practices expect_s4_class_linter,package_development best_practices +expect_true_false_and_condition_linter,package_development best_practices readability expect_type_linter,package_development best_practices extraction_operator_linter,style best_practices function_left_parentheses_linter,style readability default diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index a94e0a28b..dea56d409 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -19,6 +19,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_s3_class_linter}}} \item{\code{\link{expect_s4_class_linter}}} +\item{\code{\link{expect_true_false_and_condition_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{extraction_operator_linter}}} \item{\code{\link{implicit_integer_linter}}} diff --git a/man/expect_not_linter.Rd b/man/expect_not_linter.Rd index cb11b7baf..0331aee89 100644 --- a/man/expect_not_linter.Rd +++ b/man/expect_not_linter.Rd @@ -7,9 +7,11 @@ expect_not_linter() } \description{ -\code{\link[testthat:logical-expectations]{testthat::expect_false()}} exists specifically for testing an output is +\code{\link[testthat:logical-expectations]{testthat::expect_false()}} exists specifically for testing that an output is \code{FALSE}. \code{\link[testthat:logical-expectations]{testthat::expect_true()}} can also be used for such tests by negating the output, but it is better to use the tailored function instead. +The reverse is also true -- use \code{expect_false(A)} instead of +\code{expect_true(!A)}. } \seealso{ \link{linters} for a complete list of linters available in lintr. diff --git a/man/expect_true_false_and_condition_linter.Rd b/man/expect_true_false_and_condition_linter.Rd new file mode 100644 index 000000000..7114779e2 --- /dev/null +++ b/man/expect_true_false_and_condition_linter.Rd @@ -0,0 +1,19 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/expect_true_false_and_condition_linter.R +\name{expect_true_false_and_condition_linter} +\alias{expect_true_false_and_condition_linter} +\title{Force && conditions in expect_true(), expect_false() to be written separately} +\usage{ +expect_true_false_and_condition_linter() +} +\description{ +For readability of test outputs, testing only one thing per call to +\code{\link[testthat:logical-expectations]{testthat::expect_true()}} is preferable, i.e., +\verb{expect_true(A); expect_true(B)} is better than \code{expect_true(A && B)}. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=best_practices_linters]{best_practices}, \link[=package_development_linters]{package_development}, \link[=readability_linters]{readability} +} diff --git a/man/linters.Rd b/man/linters.Rd index c68cf9ec5..425c41a6e 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,16 +17,15 @@ Documentation for linters is structured into tags to allow for easier discovery. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (15 linters)} -\item{\link[=best_practices_linters]{best_practices} (13 linters)} +\item{\link[=best_practices_linters]{best_practices} (16 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} \item{\link[=consistency_linters]{consistency} (7 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=efficiency_linters]{efficiency} (4 linters)} -\item{\link[=package_development_linters]{package_development} (7 linters)} -\item{\link[=readability_linters]{readability} (22 linters)} +\item{\link[=package_development_linters]{package_development} (8 linters)} +\item{\link[=readability_linters]{readability} (23 linters)} \item{\link[=robustness_linters]{robustness} (10 linters)} \item{\link[=style_linters]{style} (32 linters)} } @@ -48,6 +47,7 @@ The following linters exist: \item{\code{\link{expect_null_linter}} (tags: best_practices, package_development)} \item{\code{\link{expect_s3_class_linter}} (tags: best_practices, package_development)} \item{\code{\link{expect_s4_class_linter}} (tags: best_practices, package_development)} +\item{\code{\link{expect_true_false_and_condition_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_type_linter}} (tags: best_practices, package_development)} \item{\code{\link{extraction_operator_linter}} (tags: best_practices, style)} \item{\code{\link{function_left_parentheses_linter}} (tags: default, readability, style)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index b92f6a6a5..5eb64269c 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -17,6 +17,7 @@ The following linters are tagged with 'package_development': \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_s3_class_linter}}} \item{\code{\link{expect_s4_class_linter}}} +\item{\code{\link{expect_true_false_and_condition_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{package_hooks_linter}}} } diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index 6795161e1..41ac4d1e0 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -18,6 +18,7 @@ The following linters are tagged with 'readability': \item{\code{\link{commented_code_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_not_linter}}} +\item{\code{\link{expect_true_false_and_condition_linter}}} \item{\code{\link{function_left_parentheses_linter}}} \item{\code{\link{infix_spaces_linter}}} \item{\code{\link{line_length_linter}}} diff --git a/tests/testthat/test-expect_true_false_and_condition_linter.R b/tests/testthat/test-expect_true_false_and_condition_linter.R new file mode 100644 index 000000000..1942ade18 --- /dev/null +++ b/tests/testthat/test-expect_true_false_and_condition_linter.R @@ -0,0 +1,24 @@ +test_that("expect_true_false_and_condition_linter skips allowed usages", { + expect_lint("expect_true(x)", NULL, expect_true_false_and_condition_linter()) + expect_lint("testthat::expect_false(x, y, z)", NULL, expect_true_false_and_condition_linter()) + + # more complicated expression + expect_lint("expect_true(x || (y && z))", NULL, expect_true_false_and_condition_linter()) + # the same by operator precedence, though not obvious a priori + expect_lint("expect_false(x || y && z)", NULL, expect_true_false_and_condition_linter()) + expect_lint("expect_true(x && y || z)", NULL, expect_true_false_and_condition_linter()) +}) + +test_that("expect_true_false_and_condition_linter blocks simple disallowed usages", { + expect_lint( + "expect_false(x && y)", + rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), + expect_true_false_and_condition_linter() + ) + + expect_lint( + "expect_true(x && y && z)", + rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), + expect_true_false_and_condition_linter() + ) +}) From a5f91e7aba9422bf9c57c8de22783ad430689164 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 16 Mar 2022 07:33:25 +0000 Subject: [PATCH 02/12] dedup NEWS --- NEWS.md | 1 - 1 file changed, 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 58eadd35f..476ffc063 100644 --- a/NEWS.md +++ b/NEWS.md @@ -93,7 +93,6 @@ function calls. (#850, #851, @renkun-ken) + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_ + `expect_true_false_and_condition_linter()` Require multiple `expect_true()` tests over `expect_true(x && y)` and similar - + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico) From 03785336414262b7ea00958d5e325cc2b10d1a9c Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 17 Mar 2022 22:59:39 +0000 Subject: [PATCH 03/12] rename to conjunct_expectation_linter --- DESCRIPTION | 2 +- NAMESPACE | 2 +- ...linter.R => conjunct_expectation_linter.R} | 2 +- inst/lintr/linters.csv | 2 +- man/best_practices_linters.Rd | 2 +- ...nter.Rd => conjunct_expectation_linter.Rd} | 10 ++++---- man/linters.Rd | 2 +- man/package_development_linters.Rd | 2 +- man/readability_linters.Rd | 2 +- .../test-conjunct_expectation_linter.R | 24 +++++++++++++++++++ ...t-expect_true_false_and_condition_linter.R | 24 ------------------- 11 files changed, 37 insertions(+), 37 deletions(-) rename R/{expect_true_false_and_condition_linter.R => conjunct_expectation_linter.R} (95%) rename man/{expect_true_false_and_condition_linter.Rd => conjunct_expectation_linter.Rd} (58%) create mode 100644 tests/testthat/test-conjunct_expectation_linter.R delete mode 100644 tests/testthat/test-expect_true_false_and_condition_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index d27d7d47c..a536e92a0 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -57,6 +57,7 @@ Collate: 'commas_linter.R' 'comment_linters.R' 'comments.R' + 'conjunct_expectation_linter.R' 'cyclocomp_linter.R' 'declared_functions.R' 'deprecated.R' @@ -67,7 +68,6 @@ Collate: 'expect_not_linter.R' 'expect_null_linter.R' 'expect_s3_class_linter.R' - 'expect_true_false_and_condition_linter.R' 'expect_true_false_linter.R' 'expect_type_linter.R' 'extract.R' diff --git a/NAMESPACE b/NAMESPACE index 9c05f1333..b6f59d915 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -22,6 +22,7 @@ export(clear_cache) export(closed_curly_linter) export(commas_linter) export(commented_code_linter) +export(conjunct_expectation_linter) export(cyclocomp_linter) export(default_linters) export(default_settings) @@ -35,7 +36,6 @@ export(expect_not_linter) export(expect_null_linter) export(expect_s3_class_linter) export(expect_s4_class_linter) -export(expect_true_false_and_condition_linter) export(expect_true_false_linter) export(expect_type_linter) export(extraction_operator_linter) diff --git a/R/expect_true_false_and_condition_linter.R b/R/conjunct_expectation_linter.R similarity index 95% rename from R/expect_true_false_and_condition_linter.R rename to R/conjunct_expectation_linter.R index f6ef507cf..53809535f 100644 --- a/R/expect_true_false_and_condition_linter.R +++ b/R/conjunct_expectation_linter.R @@ -7,7 +7,7 @@ #' @evalRd rd_tags("expect_true_false_and_condition_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export -expect_true_false_and_condition_linter <- function() { +conjunct_expectation_linter <- function() { Linter(function(source_file) { # need the full file to also catch usages at the top level if (length(source_file$full_parsed_content) == 0L) { diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 1e3b3aac7..ea09668c1 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -6,6 +6,7 @@ backport_linter,robustness configurable package_development closed_curly_linter,style readability default configurable commas_linter,style readability default commented_code_linter,style readability best_practices default +conjunct_expectation_linter,package_development best_practices readability cyclocomp_linter,style readability best_practices default configurable duplicate_argument_linter,correctness common_mistakes configurable equals_na_linter,robustness correctness common_mistakes default @@ -13,7 +14,6 @@ expect_not_linter,package_development best_practices readability expect_null_linter,package_development best_practices expect_s3_class_linter,package_development best_practices expect_s4_class_linter,package_development best_practices -expect_true_false_and_condition_linter,package_development best_practices readability expect_true_false_linter,package_development best_practices readability expect_type_linter,package_development best_practices extraction_operator_linter,style best_practices diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 0c1625434..72dcb48d8 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -14,12 +14,12 @@ The following linters are tagged with 'best_practices': \itemize{ \item{\code{\link{absolute_path_linter}}} \item{\code{\link{commented_code_linter}}} +\item{\code{\link{conjunct_expectation_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_s3_class_linter}}} \item{\code{\link{expect_s4_class_linter}}} -\item{\code{\link{expect_true_false_and_condition_linter}}} \item{\code{\link{expect_true_false_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{extraction_operator_linter}}} diff --git a/man/expect_true_false_and_condition_linter.Rd b/man/conjunct_expectation_linter.Rd similarity index 58% rename from man/expect_true_false_and_condition_linter.Rd rename to man/conjunct_expectation_linter.Rd index 7114779e2..147026aea 100644 --- a/man/expect_true_false_and_condition_linter.Rd +++ b/man/conjunct_expectation_linter.Rd @@ -1,10 +1,10 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/expect_true_false_and_condition_linter.R -\name{expect_true_false_and_condition_linter} -\alias{expect_true_false_and_condition_linter} +% Please edit documentation in R/conjunct_expectation_linter.R +\name{conjunct_expectation_linter} +\alias{conjunct_expectation_linter} \title{Force && conditions in expect_true(), expect_false() to be written separately} \usage{ -expect_true_false_and_condition_linter() +conjunct_expectation_linter() } \description{ For readability of test outputs, testing only one thing per call to @@ -15,5 +15,5 @@ For readability of test outputs, testing only one thing per call to \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=package_development_linters]{package_development}, \link[=readability_linters]{readability} +No tags are given. } diff --git a/man/linters.Rd b/man/linters.Rd index 8569502b9..0f8ec02ac 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -40,6 +40,7 @@ The following linters exist: \item{\code{\link{closed_curly_linter}} (tags: configurable, default, readability, style)} \item{\code{\link{commas_linter}} (tags: default, readability, style)} \item{\code{\link{commented_code_linter}} (tags: best_practices, default, readability, style)} +\item{\code{\link{conjunct_expectation_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{cyclocomp_linter}} (tags: best_practices, configurable, default, readability, style)} \item{\code{\link{duplicate_argument_linter}} (tags: common_mistakes, configurable, correctness)} \item{\code{\link{equals_na_linter}} (tags: common_mistakes, correctness, default, robustness)} @@ -47,7 +48,6 @@ The following linters exist: \item{\code{\link{expect_null_linter}} (tags: best_practices, package_development)} \item{\code{\link{expect_s3_class_linter}} (tags: best_practices, package_development)} \item{\code{\link{expect_s4_class_linter}} (tags: best_practices, package_development)} -\item{\code{\link{expect_true_false_and_condition_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_true_false_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_type_linter}} (tags: best_practices, package_development)} \item{\code{\link{extraction_operator_linter}} (tags: best_practices, style)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index b5f8f5cfa..85cb82d74 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -13,11 +13,11 @@ Linters useful to package developers, for example for writing consistent tests. The following linters are tagged with 'package_development': \itemize{ \item{\code{\link{backport_linter}}} +\item{\code{\link{conjunct_expectation_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_s3_class_linter}}} \item{\code{\link{expect_s4_class_linter}}} -\item{\code{\link{expect_true_false_and_condition_linter}}} \item{\code{\link{expect_true_false_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{package_hooks_linter}}} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index fd8971b88..af8fb07cc 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -16,9 +16,9 @@ The following linters are tagged with 'readability': \item{\code{\link{closed_curly_linter}}} \item{\code{\link{commas_linter}}} \item{\code{\link{commented_code_linter}}} +\item{\code{\link{conjunct_expectation_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_not_linter}}} -\item{\code{\link{expect_true_false_and_condition_linter}}} \item{\code{\link{expect_true_false_linter}}} \item{\code{\link{function_left_parentheses_linter}}} \item{\code{\link{infix_spaces_linter}}} diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R new file mode 100644 index 000000000..d310f0457 --- /dev/null +++ b/tests/testthat/test-conjunct_expectation_linter.R @@ -0,0 +1,24 @@ +test_that("conjunct_expectation_linter skips allowed usages", { + expect_lint("expect_true(x)", NULL, conjunct_expectation_linter()) + expect_lint("testthat::expect_false(x, y, z)", NULL, conjunct_expectation_linter()) + + # more complicated expression + expect_lint("expect_true(x || (y && z))", NULL, conjunct_expectation_linter()) + # the same by operator precedence, though not obvious a priori + expect_lint("expect_false(x || y && z)", NULL, conjunct_expectation_linter()) + expect_lint("expect_true(x && y || z)", NULL, conjunct_expectation_linter()) +}) + +test_that("conjunct_expectation_linter blocks simple disallowed usages", { + expect_lint( + "expect_false(x && y)", + rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), + conjunct_expectation_linter() + ) + + expect_lint( + "expect_true(x && y && z)", + rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), + conjunct_expectation_linter() + ) +}) diff --git a/tests/testthat/test-expect_true_false_and_condition_linter.R b/tests/testthat/test-expect_true_false_and_condition_linter.R deleted file mode 100644 index 1942ade18..000000000 --- a/tests/testthat/test-expect_true_false_and_condition_linter.R +++ /dev/null @@ -1,24 +0,0 @@ -test_that("expect_true_false_and_condition_linter skips allowed usages", { - expect_lint("expect_true(x)", NULL, expect_true_false_and_condition_linter()) - expect_lint("testthat::expect_false(x, y, z)", NULL, expect_true_false_and_condition_linter()) - - # more complicated expression - expect_lint("expect_true(x || (y && z))", NULL, expect_true_false_and_condition_linter()) - # the same by operator precedence, though not obvious a priori - expect_lint("expect_false(x || y && z)", NULL, expect_true_false_and_condition_linter()) - expect_lint("expect_true(x && y || z)", NULL, expect_true_false_and_condition_linter()) -}) - -test_that("expect_true_false_and_condition_linter blocks simple disallowed usages", { - expect_lint( - "expect_false(x && y)", - rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), - expect_true_false_and_condition_linter() - ) - - expect_lint( - "expect_true(x && y && z)", - rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), - expect_true_false_and_condition_linter() - ) -}) From 4a27969a1903d377aa9d922f368170b76f582491 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 17 Mar 2022 23:00:04 +0000 Subject: [PATCH 04/12] NEWS --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 476ffc063..3a1962a03 100644 --- a/NEWS.md +++ b/NEWS.md @@ -93,7 +93,7 @@ function calls. (#850, #851, @renkun-ken) + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_ + `expect_true_false_and_condition_linter()` Require multiple `expect_true()` tests over `expect_true(x && y)` and similar - + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar + + `conjunct_expectation_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico) # lintr 2.0.1 From 3a48d647ba6db36e815253ab7bbf4bd081f9c950 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 17 Mar 2022 23:03:33 +0000 Subject: [PATCH 05/12] decruft --- NEWS.md | 2 -- man/infix_spaces_linter.Rd | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index b6d988c99..d5970f05e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -90,8 +90,6 @@ function calls. (#850, #851, @renkun-ken) + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar + `expect_s3_class_linter()` Require usage of `expect_s3_class(x, k)` over `expect_equal(class(x), k)` and similar + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` - + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_ - + `expect_true_false_and_condition_linter()` Require multiple `expect_true()` tests over `expect_true(x && y)` and similar + `conjunct_expectation_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar diff --git a/man/infix_spaces_linter.Rd b/man/infix_spaces_linter.Rd index 3e57eadb3..50381b529 100644 --- a/man/infix_spaces_linter.Rd +++ b/man/infix_spaces_linter.Rd @@ -16,7 +16,7 @@ and that \code{=} for assignment and for setting arguments in calls are treated } \description{ Check that infix operators are surrounded by spaces. Enforces the corresponding Tidyverse style guide rule; -see \url{https://style.tidyverse.or\\href{https://g.corp.google.com/syntax}{g//syntax}.html#infix-operators}. +see \url{https://style.tidyverse.org/syntax.html#infix-operators}. } \seealso{ \link{linters} for a complete list of linters available in lintr. From 9a5f0bb647da08352d97f9f2faaa5b642253d236 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 19 Mar 2022 19:27:00 +0000 Subject: [PATCH 06/12] correct logic error re: expect_false(); NEWS --- NEWS.md | 2 +- R/conjunct_expectation_linter.R | 39 ++++++++++--------- .../test-conjunct_expectation_linter.R | 24 +++++++++--- 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/NEWS.md b/NEWS.md index d8edd264a..0ad0ff7af 100644 --- a/NEWS.md +++ b/NEWS.md @@ -90,7 +90,7 @@ function calls. (#850, #851, @renkun-ken) + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar + `expect_s3_class_linter()` Require usage of `expect_s3_class(x, k)` over `expect_equal(class(x), k)` and similar + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` - + `conjunct_expectation_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar + + `conjunct_expectation_linter()` Require usage of `expect_true(x); expect_true(y)` over `expect_equal(x && y)` and similar + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar + `expect_named_linter()` Require usage of `expect_named(x, n)` over `expect_equal(names(x), n)` and similar diff --git a/R/conjunct_expectation_linter.R b/R/conjunct_expectation_linter.R index 53809535f..4584d8c8f 100644 --- a/R/conjunct_expectation_linter.R +++ b/R/conjunct_expectation_linter.R @@ -2,7 +2,8 @@ #' #' For readability of test outputs, testing only one thing per call to #' [testthat::expect_true()] is preferable, i.e., -#' `expect_true(A); expect_true(B)` is better than `expect_true(A && B)`. +#' `expect_true(A); expect_true(B)` is better than `expect_true(A && B)`, and +#' `expect_false(A); expect_false(B)` is better than `expect_false(A || B)`. #' #' @evalRd rd_tags("expect_true_false_and_condition_linter") #' @seealso [linters] for a complete list of linters available in lintr. @@ -16,26 +17,26 @@ conjunct_expectation_linter <- function() { xml <- source_file$full_xml_parsed_content - xpath <- " - //expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'expect_false']] - /following-sibling::expr - /AND2 - " + xpath <- "//expr[ + ( + expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true']] + and expr[2][AND2] + ) or ( + expr[SYMBOL_FUNCTION_CALL[text() = 'expect_false']] + and expr[2][OR2] + ) + ]" bad_expr <- xml2::xml_find_all(xml, xpath) - return(lapply( - bad_expr, - xml_nodes_to_lint, - source_file = source_file, - message = paste( - "Write multiple && conditions in expect_true()/expect_false() as", - "individual unit tests, e.g. expect_true(x && y) becomes", - "expect_true(x) and expect_true(y). The latter will produce", - "better error messages in the case of failure." - ), - type = "warning", - global = TRUE - )) + return(lapply(bad_expr, gen_conjunct_expectation_lint, source_file)) }) } + +gen_conjunct_expectation_lint <- function(expr, source_file) { + matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL")) + op = if (matched_fun == "expect_true") "&&" else "||" + message <- sprintf("Instead of %1$s(A %2$s B), write multiple unit tests like %1$s(A) and %1$s(B)", matched_fun, op) + message <- paste(message, "The latter will produce better error messages in the case of failure.") + xml_nodes_to_lint(expr, source_file, message, type = "warning", global = TRUE) +} diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R index d310f0457..e9841c7ef 100644 --- a/tests/testthat/test-conjunct_expectation_linter.R +++ b/tests/testthat/test-conjunct_expectation_linter.R @@ -5,20 +5,34 @@ test_that("conjunct_expectation_linter skips allowed usages", { # more complicated expression expect_lint("expect_true(x || (y && z))", NULL, conjunct_expectation_linter()) # the same by operator precedence, though not obvious a priori - expect_lint("expect_false(x || y && z)", NULL, conjunct_expectation_linter()) + expect_lint("expect_true(x || y && z)", NULL, conjunct_expectation_linter()) expect_lint("expect_true(x && y || z)", NULL, conjunct_expectation_linter()) }) -test_that("conjunct_expectation_linter blocks simple disallowed usages", { +test_that("conjunct_expectation_linter blocks && conditions with expect_true()", { expect_lint( - "expect_false(x && y)", - rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), + "expect_true(x && y)", + rex::rex("Instead of expect_true(A && B), write multiple unit tests"), conjunct_expectation_linter() ) expect_lint( "expect_true(x && y && z)", - rex::rex("Write multiple && conditions in expect_true()/expect_false() as"), + rex::rex("Instead of expect_true(A && B), write multiple unit tests"), + conjunct_expectation_linter() + ) +}) + +test_that("conjunct_expectation_linter blocks || conditions with expect_false()", { + expect_lint( + "expect_false(x || y)", + rex::rex("Instead of expect_false(A || B), write multiple unit tests"), + conjunct_expectation_linter() + ) + + expect_lint( + "expect_false(x || y || z)", + rex::rex("Instead of expect_false(A || B), write multiple unit tests"), conjunct_expectation_linter() ) }) From 3b3467615638f6b2d5c601b1d519bc1239131653 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 20 Mar 2022 05:39:36 +0000 Subject: [PATCH 07/12] NEWS typo --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 0ad0ff7af..ab250ce5a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -90,7 +90,7 @@ function calls. (#850, #851, @renkun-ken) + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar + `expect_s3_class_linter()` Require usage of `expect_s3_class(x, k)` over `expect_equal(class(x), k)` and similar + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` - + `conjunct_expectation_linter()` Require usage of `expect_true(x); expect_true(y)` over `expect_equal(x && y)` and similar + + `conjunct_expectation_linter()` Require usage of `expect_true(x); expect_true(y)` over `expect_true(x && y)` and similar + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar + `expect_named_linter()` Require usage of `expect_named(x, n)` over `expect_equal(names(x), n)` and similar From edd858ab47f072a0e3cef3caa676d8e1b0a86c52 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 20 Mar 2022 05:41:03 +0000 Subject: [PATCH 08/12] fix terminology --- R/conjunct_expectation_linter.R | 2 +- tests/testthat/test-conjunct_expectation_linter.R | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/R/conjunct_expectation_linter.R b/R/conjunct_expectation_linter.R index 4584d8c8f..8c4c2d0d0 100644 --- a/R/conjunct_expectation_linter.R +++ b/R/conjunct_expectation_linter.R @@ -36,7 +36,7 @@ conjunct_expectation_linter <- function() { gen_conjunct_expectation_lint <- function(expr, source_file) { matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL")) op = if (matched_fun == "expect_true") "&&" else "||" - message <- sprintf("Instead of %1$s(A %2$s B), write multiple unit tests like %1$s(A) and %1$s(B)", matched_fun, op) + message <- sprintf("Instead of %1$s(A %2$s B), write multiple expectations like %1$s(A) and %1$s(B)", matched_fun, op) message <- paste(message, "The latter will produce better error messages in the case of failure.") xml_nodes_to_lint(expr, source_file, message, type = "warning", global = TRUE) } diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R index e9841c7ef..ec26a5b0d 100644 --- a/tests/testthat/test-conjunct_expectation_linter.R +++ b/tests/testthat/test-conjunct_expectation_linter.R @@ -12,13 +12,13 @@ test_that("conjunct_expectation_linter skips allowed usages", { test_that("conjunct_expectation_linter blocks && conditions with expect_true()", { expect_lint( "expect_true(x && y)", - rex::rex("Instead of expect_true(A && B), write multiple unit tests"), + rex::rex("Instead of expect_true(A && B), write multiple expectations"), conjunct_expectation_linter() ) expect_lint( "expect_true(x && y && z)", - rex::rex("Instead of expect_true(A && B), write multiple unit tests"), + rex::rex("Instead of expect_true(A && B), write multiple expectations"), conjunct_expectation_linter() ) }) @@ -26,13 +26,13 @@ test_that("conjunct_expectation_linter blocks && conditions with expect_true()", test_that("conjunct_expectation_linter blocks || conditions with expect_false()", { expect_lint( "expect_false(x || y)", - rex::rex("Instead of expect_false(A || B), write multiple unit tests"), + rex::rex("Instead of expect_false(A || B), write multiple expectations"), conjunct_expectation_linter() ) expect_lint( "expect_false(x || y || z)", - rex::rex("Instead of expect_false(A || B), write multiple unit tests"), + rex::rex("Instead of expect_false(A || B), write multiple expectations"), conjunct_expectation_linter() ) }) From ab5d74c40dbe055b5e49a0e37399daa5a865e68d Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 20 Mar 2022 05:47:21 +0000 Subject: [PATCH 09/12] extend tests --- tests/testthat/test-conjunct_expectation_linter.R | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R index ec26a5b0d..61462e683 100644 --- a/tests/testthat/test-conjunct_expectation_linter.R +++ b/tests/testthat/test-conjunct_expectation_linter.R @@ -1,12 +1,22 @@ -test_that("conjunct_expectation_linter skips allowed usages", { +test_that("conjunct_expectation_linter skips allowed usages of expect_true", { expect_lint("expect_true(x)", NULL, conjunct_expectation_linter()) - expect_lint("testthat::expect_false(x, y, z)", NULL, conjunct_expectation_linter()) + expect_lint("testthat::expect_true(x, y, z)", NULL, conjunct_expectation_linter()) # more complicated expression expect_lint("expect_true(x || (y && z))", NULL, conjunct_expectation_linter()) # the same by operator precedence, though not obvious a priori expect_lint("expect_true(x || y && z)", NULL, conjunct_expectation_linter()) expect_lint("expect_true(x && y || z)", NULL, conjunct_expectation_linter()) + +}) + +test_that("conjunct_expectation_linter skips allowed usages of expect_true", { + expect_lint("expect_false(x)", NULL, conjunct_expectation_linter()) + expect_lint("testthat::expect_false(x, y, z)", NULL, conjunct_expectation_linter()) + + # more complicated expression + # (NB: xx && yy || zz and xx || yy && zz both parse with || first) + expect_lint("expect_false(x && (y || z))", NULL, conjunct_expectation_linter()) }) test_that("conjunct_expectation_linter blocks && conditions with expect_true()", { From 2203451942afb3efd408ec66d5107c9afe0f8141 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 21 Mar 2022 17:56:00 +0000 Subject: [PATCH 10/12] <- not = --- R/conjunct_expectation_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/conjunct_expectation_linter.R b/R/conjunct_expectation_linter.R index 8c4c2d0d0..183e5a51e 100644 --- a/R/conjunct_expectation_linter.R +++ b/R/conjunct_expectation_linter.R @@ -35,7 +35,7 @@ conjunct_expectation_linter <- function() { gen_conjunct_expectation_lint <- function(expr, source_file) { matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL")) - op = if (matched_fun == "expect_true") "&&" else "||" + op <- if (matched_fun == "expect_true") "&&" else "||" message <- sprintf("Instead of %1$s(A %2$s B), write multiple expectations like %1$s(A) and %1$s(B)", matched_fun, op) message <- paste(message, "The latter will produce better error messages in the case of failure.") xml_nodes_to_lint(expr, source_file, message, type = "warning", global = TRUE) From 42069941a9a388afae3e3793425b60fba514e603 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 21 Mar 2022 17:56:09 +0000 Subject: [PATCH 11/12] add operator precedence tests --- tests/testthat/test-conjunct_expectation_linter.R | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R index 61462e683..e14cc4bec 100644 --- a/tests/testthat/test-conjunct_expectation_linter.R +++ b/tests/testthat/test-conjunct_expectation_linter.R @@ -7,7 +7,6 @@ test_that("conjunct_expectation_linter skips allowed usages of expect_true", { # the same by operator precedence, though not obvious a priori expect_lint("expect_true(x || y && z)", NULL, conjunct_expectation_linter()) expect_lint("expect_true(x && y || z)", NULL, conjunct_expectation_linter()) - }) test_that("conjunct_expectation_linter skips allowed usages of expect_true", { @@ -45,4 +44,16 @@ test_that("conjunct_expectation_linter blocks || conditions with expect_false()" rex::rex("Instead of expect_false(A || B), write multiple expectations"), conjunct_expectation_linter() ) + + # these lint because `||` is always outer by operator precedence + expect_lint( + "expect_false(x || y && z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_expectation_linter() + ) + expect_lint( + "expect_false(x && y || z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_expectation_linter() + ) }) From bcc1d412097d8fa775ec465d96932f660ab13698 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 21 Mar 2022 17:58:52 +0000 Subject: [PATCH 12/12] use new xml_nodes_to_lint --- R/conjunct_expectation_linter.R | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/R/conjunct_expectation_linter.R b/R/conjunct_expectation_linter.R index 183e5a51e..222ea8dcd 100644 --- a/R/conjunct_expectation_linter.R +++ b/R/conjunct_expectation_linter.R @@ -29,14 +29,19 @@ conjunct_expectation_linter <- function() { bad_expr <- xml2::xml_find_all(xml, xpath) - return(lapply(bad_expr, gen_conjunct_expectation_lint, source_file)) + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file, + lint_message = function(expr) { + matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL")) + op <- if (matched_fun == "expect_true") "&&" else "||" + message <- + sprintf("Instead of %1$s(A %2$s B), write multiple expectations like %1$s(A) and %1$s(B)", matched_fun, op) + paste(message, "The latter will produce better error messages in the case of failure.") + }, + type = "warning", + global = TRUE + )) }) } - -gen_conjunct_expectation_lint <- function(expr, source_file) { - matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL")) - op <- if (matched_fun == "expect_true") "&&" else "||" - message <- sprintf("Instead of %1$s(A %2$s B), write multiple expectations like %1$s(A) and %1$s(B)", matched_fun, op) - message <- paste(message, "The latter will produce better error messages in the case of failure.") - xml_nodes_to_lint(expr, source_file, message, type = "warning", global = TRUE) -}