-
Notifications
You must be signed in to change notification settings - Fork 186
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Split S3 and S4 linters (and tests) into their own files (#1680)
* Split S3 and S4 linters (and tests) into their own files This always made me do double-take if I was working in the right file. * correct links
- Loading branch information
1 parent
849c40e
commit 719784c
Showing
7 changed files
with
100 additions
and
91 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| #' Require usage of `expect_s4_class(x, k)` over `expect_true(is(x, k))` | ||
| #' | ||
| #' [testthat::expect_s4_class()] exists specifically for testing the class | ||
| #' of S4 objects. [testthat::expect_true()] can also be used for such tests, | ||
| #' but it is better to use the tailored function instead. | ||
| #' | ||
| #' @examples | ||
| #' # will produce lints | ||
| #' lint( | ||
| #' text = "expect_true(is(x, 'Matrix'))", | ||
| #' linters = expect_s4_class_linter() | ||
| #' ) | ||
| #' | ||
| #' # okay | ||
| #' lint( | ||
| #' text = "expect_s4_class(x, 'Matrix')", | ||
| #' linters = expect_s4_class_linter() | ||
| #' ) | ||
| #' | ||
| #' @evalRd rd_tags("expect_s4_class_linter") | ||
| #' @seealso [linters] for a complete list of linters available in lintr. \cr | ||
| #' [expect_s3_class_linter()] | ||
| #' @export | ||
| expect_s4_class_linter <- function() { | ||
| # require 2 expressions because methods::is(x) alone is a valid call, even | ||
| # though the character output wouldn't make any sense for expect_true(). | ||
| xpath <- " | ||
| //SYMBOL_FUNCTION_CALL[text() = 'expect_true'] | ||
| /parent::expr | ||
| /following-sibling::expr[1][count(expr) = 3 and expr[1][SYMBOL_FUNCTION_CALL[text() = 'is']]] | ||
| /parent::expr[not(SYMBOL_SUB[text() = 'info' or text() = 'label'])] | ||
| " | ||
|
|
||
| Linter(function(source_expression) { | ||
| if (!is_lint_level(source_expression, "expression")) { | ||
| return(list()) | ||
| } | ||
|
|
||
| xml <- source_expression$xml_parsed_content | ||
|
|
||
| # TODO(michaelchirico): also catch expect_{equal,identical}(methods::is(x), k). | ||
| # this seems empirically rare, but didn't check many S4-heavy packages. | ||
|
|
||
| bad_expr <- xml2::xml_find_all(xml, xpath) | ||
| xml_nodes_to_lints( | ||
| bad_expr, | ||
| source_expression = source_expression, | ||
| lint_message = paste( | ||
| "expect_s4_class(x, k) is better than expect_true(is(x, k)).", | ||
| "Note also expect_s3_class() available for testing S3 objects." | ||
| ), | ||
| type = "warning" | ||
| ) | ||
| }) | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| test_that("expect_s4_class_linter skips allowed usages", { | ||
| linter <- expect_s4_class_linter() | ||
|
|
||
| # expect_s4_class doesn't have an inverted version | ||
| expect_lint("expect_true(!is(x, 'class'))", NULL, linter) | ||
| # NB: also applies to tinytest, but it's sufficient to test testthat | ||
| expect_lint("testthat::expect_s3_class(!is(x, 'class'))", NULL, linter) | ||
|
|
||
| # expect_s4_class() doesn't have info= or label= arguments | ||
| expect_lint("expect_true(is(x, 'SpatialPoly'), info = 'x should be SpatialPoly')", NULL, linter) | ||
| expect_lint("expect_true(is(x, 'SpatialPoly'), label = 'x inheritance')", NULL, linter) | ||
| }) | ||
|
|
||
| test_that("expect_s4_class blocks simple disallowed usages", { | ||
| expect_lint( | ||
| "expect_true(is(x, 'data.frame'))", | ||
| rex::rex("expect_s4_class(x, k) is better than expect_true(is(x, k))"), | ||
| expect_s4_class_linter() | ||
| ) | ||
|
|
||
| # namespace qualification is irrelevant | ||
| expect_lint( | ||
| "testthat::expect_true(methods::is(x, 'SpatialPolygonsDataFrame'))", | ||
| rex::rex("expect_s4_class(x, k) is better than expect_true(is(x, k))"), | ||
| expect_s4_class_linter() | ||
| ) | ||
| }) |