From e53a2c8bf70f985b272c84e45143bbbad63d22ce Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 17 Mar 2022 19:59:23 -0700 Subject: [PATCH] New expect_named_linter (#949) * New expect_named_linter * NEWS * fix lints in lintr --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 1 + R/expect_named_linter.R | 35 ++++++++++++++++++ inst/lintr/linters.csv | 1 + man/best_practices_linters.Rd | 1 + man/expect_named_linter.Rd | 19 ++++++++++ man/linters.Rd | 1 + man/package_development_linters.Rd | 1 + man/readability_linters.Rd | 1 + tests/testthat/test-expect_named_linter.R | 44 +++++++++++++++++++++++ tests/testthat/test-settings.R | 12 +++---- 12 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 R/expect_named_linter.R create mode 100644 man/expect_named_linter.Rd create mode 100644 tests/testthat/test-expect_named_linter.R diff --git a/DESCRIPTION b/DESCRIPTION index 5f3f90d33..a94a97be0 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -64,6 +64,7 @@ Collate: 'exclude.R' 'expect_length_linter.R' 'expect_lint.R' + 'expect_named_linter.R' 'expect_not_linter.R' 'expect_null_linter.R' 'expect_s3_class_linter.R' diff --git a/NAMESPACE b/NAMESPACE index d86c70569..957f6fcf5 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -31,6 +31,7 @@ export(equals_na_linter) export(expect_length_linter) export(expect_lint) export(expect_lint_free) +export(expect_named_linter) export(expect_not_linter) export(expect_null_linter) export(expect_s3_class_linter) diff --git a/NEWS.md b/NEWS.md index cc17577c6..1f6616efd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -92,6 +92,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_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 * `expect_length_linter()` Require usage of `expect_length(x, n)` over `expect_equal(length(x), n)` 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) * `infix_spaces_linter()` gains argument `exclude_operators` to disable lints on selected infix operators. By default, all "low-precedence" operators throw lints; see `?infix_spaces_linter` for an enumeration of these. (#914 @michaelchirico) diff --git a/R/expect_named_linter.R b/R/expect_named_linter.R new file mode 100644 index 000000000..59313b519 --- /dev/null +++ b/R/expect_named_linter.R @@ -0,0 +1,35 @@ +#' Require usage of expect_named(x, n) over expect_equal(names(x), n) +#' +#' [testthat::expect_named()] exists specifically for testing the [names()] of +#' an object. [testthat::expect_equal()] can also be used for such tests, +#' but it is better to use the tailored function instead. +#' +#' @evalRd rd_tags("expect_named_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +expect_named_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_equal' or text() = 'expect_identical'] + and following-sibling::expr[ + expr[SYMBOL_FUNCTION_CALL[text() = 'names']] + and (position() = 1 or preceding-sibling::expr[STR_CONST]) + ] + ]" + + bad_expr <- xml2::xml_find_all(xml, xpath) + return(lapply(bad_expr, gen_expect_named_lint, source_file)) + }) +} + +gen_expect_named_lint <- function(expr, source_file) { + matched_function <- xml2::xml_text(xml2::xml_find_first(expr, "SYMBOL_FUNCTION_CALL")) + lint_msg <- sprintf("expect_named(x, n) is better than %s(names(x), n)", matched_function) + xml_nodes_to_lint(expr, source_file, lint_msg, type = "warning") +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 44410f9a3..6284d97fc 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -9,6 +9,7 @@ cyclocomp_linter,style readability best_practices default configurable duplicate_argument_linter,correctness common_mistakes configurable equals_na_linter,robustness correctness common_mistakes default expect_length_linter,package_development best_practices readability +expect_named_linter,package_development best_practices readability expect_not_linter,package_development best_practices readability expect_null_linter,package_development best_practices expect_s3_class_linter,package_development best_practices diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index 65d489330..db6c374c5 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -16,6 +16,7 @@ The following linters are tagged with 'best_practices': \item{\code{\link{commented_code_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_length_linter}}} +\item{\code{\link{expect_named_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_s3_class_linter}}} diff --git a/man/expect_named_linter.Rd b/man/expect_named_linter.Rd new file mode 100644 index 000000000..4f1a442b8 --- /dev/null +++ b/man/expect_named_linter.Rd @@ -0,0 +1,19 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/expect_named_linter.R +\name{expect_named_linter} +\alias{expect_named_linter} +\title{Require usage of expect_named(x, n) over expect_equal(names(x), n)} +\usage{ +expect_named_linter() +} +\description{ +\code{\link[testthat:expect_named]{testthat::expect_named()}} exists specifically for testing the \code{\link[=names]{names()}} of +an object. \code{\link[testthat:equality-expectations]{testthat::expect_equal()}} can also be used for such tests, +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 bbce28306..42168d448 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -43,6 +43,7 @@ The following linters exist: \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_length_linter}} (tags: best_practices, package_development, readability)} +\item{\code{\link{expect_named_linter}} (tags: best_practices, package_development, readability)} \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_s3_class_linter}} (tags: best_practices, package_development)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index 2b736cc24..3f7510b42 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -14,6 +14,7 @@ The following linters are tagged with 'package_development': \itemize{ \item{\code{\link{backport_linter}}} \item{\code{\link{expect_length_linter}}} +\item{\code{\link{expect_named_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_null_linter}}} \item{\code{\link{expect_s3_class_linter}}} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index a9e39045a..2236178af 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{commented_code_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_length_linter}}} +\item{\code{\link{expect_named_linter}}} \item{\code{\link{expect_not_linter}}} \item{\code{\link{expect_true_false_linter}}} \item{\code{\link{function_left_parentheses_linter}}} diff --git a/tests/testthat/test-expect_named_linter.R b/tests/testthat/test-expect_named_linter.R new file mode 100644 index 000000000..adfeaab33 --- /dev/null +++ b/tests/testthat/test-expect_named_linter.R @@ -0,0 +1,44 @@ +test_that("expect_named_linter doesn't raise false positive lints", { + expect_lint("expect_equal(nrow(x), 4L)", NULL, expect_named_linter()) + # NB: also applies to tinytest, but it's sufficient to test testthat + expect_lint("testthat::expect_equal(nrow(x), 4L)", NULL, expect_named_linter()) +}) + +test_that("expect_named_linter skips allowed usages", { + # colnames(), rownames(), and dimnames() tests are not equivalent + expect_lint("expect_equal(colnames(x), 'a')", NULL, expect_named_linter()) + expect_lint("expect_equal(rownames(x), 'a')", NULL, expect_named_linter()) + expect_lint("expect_equal(dimnames(x), 'a')", NULL, expect_named_linter()) + + # only check the first argument. yoda tests in the second argument will be + # missed, but there are legitimate uses of names() in argument 2 + expect_lint("expect_equal(colnames(x), names(y))", NULL, expect_named_linter()) +}) + +test_that("expect_named_linter blocks simple disallowed usages", { + expect_lint( + "expect_equal(names(x), 'a')", + rex::rex("expect_named(x, n) is better than expect_equal(names(x), n)"), + expect_named_linter() + ) + + expect_lint( + "testthat::expect_equal(names(DF), names(old))", + rex::rex("expect_named(x, n) is better than expect_equal(names(x), n)"), + expect_named_linter() + ) + + expect_lint( + "expect_equal('a', names(x))", + rex::rex("expect_named(x, n) is better than expect_equal(names(x), n)"), + expect_named_linter() + ) +}) + +test_that("expect_named_linter blocks expect_identical usage as well", { + expect_lint( + "expect_identical(names(x), 'a')", + rex::rex("expect_named(x, n) is better than expect_identical(names(x), n)"), + expect_named_linter() + ) +}) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 3d7d0b31a..e3b944a2c 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -74,11 +74,9 @@ test_that("it errors if the config file does not end in a newline", { expect_error(read_settings("foo"), "Malformed config file") }) -test_that("with_defaults works as expected", { - # test capturing unnamed args - defaults <- with_defaults(assignment_linter) +test_that("with_defaults works as expected with unnamed args", { # assignment_linter is in defaults, so output doesn't change - expect_equal(names(defaults), names(with_defaults())) + expect_named(with_defaults(assignment_linter), names(with_defaults())) }) test_that("rot utility works as intended", { @@ -106,8 +104,8 @@ test_that("logical_env utility works as intended", { # fixing #774 test_that("with_defaults doesn't break on very long input", { - expect_equal( - names(with_defaults( + expect_named( + with_defaults( default = list(), lintr::undesirable_function_linter(c( detach = paste( @@ -120,7 +118,7 @@ test_that("with_defaults doesn't break on very long input", { "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" ) )) - )), + ), "lintr::undesirable_function_linter" ) })