diff --git a/DESCRIPTION b/DESCRIPTION index 725ee0b46..83ce0801d 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -60,7 +60,7 @@ Collate: 'commas_linter.R' 'comment_linters.R' 'comments.R' - 'conjunct_expectation_linter.R' + 'conjunct_test_linter.R' 'consecutive_stopifnot_linter.R' 'cyclocomp_linter.R' 'declared_functions.R' diff --git a/NAMESPACE b/NAMESPACE index 23ccda32f..267f416bf 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -24,7 +24,7 @@ export(clear_cache) export(closed_curly_linter) export(commas_linter) export(commented_code_linter) -export(conjunct_expectation_linter) +export(conjunct_test_linter) export(consecutive_stopifnot_linter) export(cyclocomp_linter) export(default_linters) diff --git a/NEWS.md b/NEWS.md index d6a1fc124..a8618995f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -99,7 +99,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_true(x && y)` and similar + + `conjunct_test_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 diff --git a/R/conjunct_expectation_linter.R b/R/conjunct_test_linter.R similarity index 57% rename from R/conjunct_expectation_linter.R rename to R/conjunct_test_linter.R index 222ea8dcd..c92d18350 100644 --- a/R/conjunct_expectation_linter.R +++ b/R/conjunct_test_linter.R @@ -5,10 +5,10 @@ #' `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") +#' @evalRd rd_tags("conjunct_test_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export -conjunct_expectation_linter <- function() { +conjunct_test_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) { @@ -17,9 +17,11 @@ conjunct_expectation_linter <- function() { xml <- source_file$full_xml_parsed_content + # TODO(#1016): include assert_that() for consideration here too + # TODO(#1017): address keyword arguments in R>=4.0 for stopifnot(). lint optionally (on by default)? xpath <- "//expr[ ( - expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true']] + expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'stopifnot']] and expr[2][AND2] ) or ( expr[SYMBOL_FUNCTION_CALL[text() = 'expect_false']] @@ -35,10 +37,14 @@ conjunct_expectation_linter <- function() { 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.") + op <- xml2::xml_text(xml2::xml_find_first(expr, "expr/*[self::AND2 or self::OR2]")) + instead_of <- sprintf("Instead of %s(A %s B),", matched_fun, op) + if (matched_fun %in% c("expect_true", "expect_false")) { + replacement <- sprintf("write multiple expectations like %1$s(A) and %1$s(B)", matched_fun) + } else { + replacement <- "write multiple conditions like stopifnot(A, B)." + } + paste(instead_of, replacement, "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 749fc291e..939a86af0 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -8,7 +8,7 @@ class_equals_linter,best_practices robustness consistency 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 +conjunct_test_linter,package_development best_practices readability consecutive_stopifnot_linter,style readability consistency cyclocomp_linter,style readability best_practices default configurable duplicate_argument_linter,correctness common_mistakes configurable diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index c8f3cd81b..4cb3930bf 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -17,7 +17,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{any_is_na_linter}}} \item{\code{\link{class_equals_linter}}} \item{\code{\link{commented_code_linter}}} -\item{\code{\link{conjunct_expectation_linter}}} +\item{\code{\link{conjunct_test_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_comparison_linter}}} \item{\code{\link{expect_length_linter}}} diff --git a/man/conjunct_expectation_linter.Rd b/man/conjunct_test_linter.Rd similarity index 67% rename from man/conjunct_expectation_linter.Rd rename to man/conjunct_test_linter.Rd index 1125e999e..4fc207726 100644 --- a/man/conjunct_expectation_linter.Rd +++ b/man/conjunct_test_linter.Rd @@ -1,10 +1,10 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/conjunct_expectation_linter.R -\name{conjunct_expectation_linter} -\alias{conjunct_expectation_linter} +% Please edit documentation in R/conjunct_test_linter.R +\name{conjunct_test_linter} +\alias{conjunct_test_linter} \title{Force && conditions in expect_true(), expect_false() to be written separately} \usage{ -conjunct_expectation_linter() +conjunct_test_linter() } \description{ For readability of test outputs, testing only one thing per call to @@ -16,5 +16,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}{ -No tags are given. +\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 21ed55af2..f99ef5b0a 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,13 +17,13 @@ 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} (32 linters)} +\item{\link[=best_practices_linters]{best_practices} (33 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} \item{\link[=consistency_linters]{consistency} (14 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (28 linters)} -\item{\link[=efficiency_linters]{efficiency} (12 linters)} +\item{\link[=efficiency_linters]{efficiency} (13 linters)} \item{\link[=package_development_linters]{package_development} (14 linters)} \item{\link[=readability_linters]{readability} (35 linters)} \item{\link[=robustness_linters]{robustness} (11 linters)} @@ -42,7 +42,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{conjunct_test_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{consecutive_stopifnot_linter}} (tags: consistency, readability, style)} \item{\code{\link{cyclocomp_linter}} (tags: best_practices, configurable, default, readability, style)} \item{\code{\link{duplicate_argument_linter}} (tags: common_mistakes, configurable, correctness)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index fe8d9b7d3..fabe3b703 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -13,7 +13,7 @@ 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{conjunct_test_linter}}} \item{\code{\link{expect_comparison_linter}}} \item{\code{\link{expect_identical_linter}}} \item{\code{\link{expect_length_linter}}} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index fdfb9b749..2b2ec92dd 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -15,7 +15,7 @@ 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{conjunct_test_linter}}} \item{\code{\link{consecutive_stopifnot_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{else_same_line_linter}}} diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R deleted file mode 100644 index e14cc4bec..000000000 --- a/tests/testthat/test-conjunct_expectation_linter.R +++ /dev/null @@ -1,59 +0,0 @@ -test_that("conjunct_expectation_linter skips allowed usages of expect_true", { - expect_lint("expect_true(x)", 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()", { - expect_lint( - "expect_true(x && y)", - 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 expectations"), - 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 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() - ) - - # 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() - ) -}) diff --git a/tests/testthat/test-conjunct_test_linter.R b/tests/testthat/test-conjunct_test_linter.R new file mode 100644 index 000000000..f07a7395b --- /dev/null +++ b/tests/testthat/test-conjunct_test_linter.R @@ -0,0 +1,84 @@ +test_that("conjunct_test_linter skips allowed usages of expect_true", { + expect_lint("expect_true(x)", NULL, conjunct_test_linter()) + expect_lint("testthat::expect_true(x, y, z)", NULL, conjunct_test_linter()) + + # more complicated expression + expect_lint("expect_true(x || (y && z))", NULL, conjunct_test_linter()) + # the same by operator precedence, though not obvious a priori + expect_lint("expect_true(x || y && z)", NULL, conjunct_test_linter()) + expect_lint("expect_true(x && y || z)", NULL, conjunct_test_linter()) +}) + +test_that("conjunct_test_linter skips allowed usages of expect_true", { + expect_lint("expect_false(x)", NULL, conjunct_test_linter()) + expect_lint("testthat::expect_false(x, y, z)", NULL, conjunct_test_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_test_linter()) +}) + +test_that("conjunct_test_linter blocks && conditions with expect_true()", { + expect_lint( + "expect_true(x && y)", + rex::rex("Instead of expect_true(A && B), write multiple expectations"), + conjunct_test_linter() + ) + + expect_lint( + "expect_true(x && y && z)", + rex::rex("Instead of expect_true(A && B), write multiple expectations"), + conjunct_test_linter() + ) +}) + +test_that("conjunct_test_linter blocks || conditions with expect_false()", { + expect_lint( + "expect_false(x || y)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_test_linter() + ) + + expect_lint( + "expect_false(x || y || z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_test_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_test_linter() + ) + expect_lint( + "expect_false(x && y || z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_test_linter() + ) +}) + +test_that("conjunct_test_linter skips allowed usages", { + expect_lint("stopifnot(x)", NULL, conjunct_test_linter()) + expect_lint("stopifnot(x, y, z)", NULL, conjunct_test_linter()) + + # more complicated expression + expect_lint("stopifnot(x || (y && z))", NULL, conjunct_test_linter()) + # the same by operator precedence, though not obvious a priori + expect_lint("stopifnot(x || y && z)", NULL, conjunct_test_linter()) + expect_lint("stopifnot(x && y || z)", NULL, conjunct_test_linter()) +}) + +test_that("conjunct_stopifnot_linter blocks simple disallowed usages", { + expect_lint( + "stopifnot(x && y)", + rex::rex("Instead of stopifnot(A && B), write multiple conditions"), + conjunct_test_linter() + ) + + expect_lint( + "stopifnot(x && y && z)", + rex::rex("Instead of stopifnot(A && B), write multiple conditions"), + conjunct_test_linter() + ) +})