diff --git a/DESCRIPTION b/DESCRIPTION index 5be2bb529..4e44760c6 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -64,6 +64,7 @@ Collate: 'equals_na_linter.R' 'exclude.R' 'expect_lint.R' + 'expect_not_linter.R' 'expect_null_linter.R' 'expect_type_linter.R' 'extract.R' diff --git a/NAMESPACE b/NAMESPACE index fe9d2a911..d563fed50 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -31,6 +31,7 @@ export(duplicate_argument_linter) export(equals_na_linter) export(expect_lint) export(expect_lint_free) +export(expect_not_linter) export(expect_null_linter) export(expect_type_linter) export(extraction_operator_linter) diff --git a/NEWS.md b/NEWS.md index 0cd733698..64ae49a9f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -89,6 +89,7 @@ function calls. (#850, #851, @renkun-ken) * `lintr` is adopting a new set of linters provided as part of Google's extension to the tidyverse style guide (#884, @michaelchirico) + `expect_null_linter()` Require usage of `expect_null(x)` over `expect_equal(x, NULL)` and similar + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar + + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. * `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_not_linter.R b/R/expect_not_linter.R new file mode 100644 index 000000000..c7e3d85d3 --- /dev/null +++ b/R/expect_not_linter.R @@ -0,0 +1,35 @@ +#' Require usage of expect_false(.) over expect_true(!.) +#' +#' [testthat::expect_false()] exists specifically for testing that an output is +#' `FALSE`. [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 `expect_false(A)` instead of +#' `expect_true(!A)`. +#' +#' @evalRd rd_tags("expect_not_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +expect_not_linter <- function() { + Linter(function(source_file) { + if (length(source_file$parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$xml_parsed_content + + xpath <- "//expr[ + SYMBOL_FUNCTION_CALL[text() = 'expect_true' or text() = 'expect_false'] + and following-sibling::expr[1][OP-EXCLAMATION] + ]" + + bad_expr <- xml2::xml_find_all(xml, xpath) + + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file = source_file, + message = "expect_false(x) is better than expect_true(!x), and vice versa.", + type = "warning" + )) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index be4751581..ce0dcbf02 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -9,6 +9,7 @@ commented_code_linter,style readability best_practices default cyclocomp_linter,style readability best_practices default configurable duplicate_argument_linter,correctness common_mistakes configurable equals_na_linter,robustness correctness common_mistakes default +expect_not_linter,package_development best_practices readability expect_null_linter,package_development best_practices 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 38b823abb..b84a09504 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -15,6 +15,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{absolute_path_linter}}} \item{\code{\link{commented_code_linter}}} \item{\code{\link{cyclocomp_linter}}} +\item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_type_linter}}} \item{\code{\link{extraction_operator_linter}}} diff --git a/man/expect_not_linter.Rd b/man/expect_not_linter.Rd new file mode 100644 index 000000000..cb11b7baf --- /dev/null +++ b/man/expect_not_linter.Rd @@ -0,0 +1,19 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/expect_not_linter.R +\name{expect_not_linter} +\alias{expect_not_linter} +\title{Require usage of expect_false(.) over expect_true(!.)} +\usage{ +expect_not_linter() +} +\description{ +\code{\link[testthat:logical-expectations]{testthat::expect_false()}} exists specifically for testing 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. +} +\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 d36397bff..6c3353f97 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,15 +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} (12 linters)} +\item{\link[=best_practices_linters]{best_practices} (13 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} (4 linters)} -\item{\link[=readability_linters]{readability} (21 linters)} +\item{\link[=package_development_linters]{package_development} (5 linters)} +\item{\link[=readability_linters]{readability} (22 linters)} \item{\link[=robustness_linters]{robustness} (10 linters)} \item{\link[=style_linters]{style} (32 linters)} } @@ -43,6 +43,7 @@ The following linters exist: \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)} +\item{\code{\link{expect_not_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_null_linter}} (tags: best_practices, package_development)} \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 2ced927e3..6fbc2e23b 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -13,6 +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{expect_not_linter}}} \item{\code{\link{expect_null_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 4e9be19c7..6795161e1 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -17,6 +17,7 @@ The following linters are tagged with 'readability': \item{\code{\link{commas_linter}}} \item{\code{\link{commented_code_linter}}} \item{\code{\link{cyclocomp_linter}}} +\item{\code{\link{expect_not_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_not_linter.R b/tests/testthat/test-expect_not_linter.R new file mode 100644 index 000000000..7f7defe14 --- /dev/null +++ b/tests/testthat/test-expect_not_linter.R @@ -0,0 +1,37 @@ +test_that("expect_not_linter skips allowed usages", { + expect_lint("expect_true(x)", NULL, expect_not_linter()) + # NB: also applies to tinytest, but it's sufficient to test testthat + expect_lint("testthat::expect_true(x)", NULL, expect_not_linter()) + expect_lint("expect_false(x)", NULL, expect_not_linter()) + expect_lint("testthat::expect_false(x)", NULL, expect_not_linter()) + + # not a strict ban on ! + ## (expect_false(x && y) is the same, but it's not clear which to prefer) + expect_lint("expect_true(!x || !y)", NULL, expect_not_linter()) +}) + +test_that("expect_not_linter blocks simple disallowed usages", { + expect_lint( + "expect_true(!x)", + "expect_false\\(x\\) is better than expect_true\\(!x\\)", + expect_not_linter() + ) + + expect_lint( + "testthat::expect_true(!x)", + "expect_false\\(x\\) is better than expect_true\\(!x\\)", + expect_not_linter() + ) + + expect_lint( + "expect_false(!foo(x))", + "expect_false\\(x\\) is better than expect_true\\(!x\\)", + expect_not_linter() + ) + + expect_lint( + "testthat::expect_true(!(x && y))", + "expect_false\\(x\\) is better than expect_true\\(!x\\)", + expect_not_linter() + ) +})