expect_s3_class_linter and expect_s4_class_linter

[MichaelChirico]

Part of #884

Close cousin of expect_type_linter()

[MichaelChirico]

BTW, one thing to consider -- with this linter active, it becomes somewhat cumbersome to test for the exact sequence of S3 classes, e.g. before you might write

expect_equal(class(DT), c("data.table", "data.frame"))

this linter wants

expect_s3_class(DT, "data.table")
expect_s3_class(DT, "data.frame")

The thinking is that it's usually preferable to check inheritance as opposed to an exact sequence of classes.

One thing we could do is offer an option (I'm indifferent to the default) lint_identical that toggles whether to lint expect_identical(). If it's turned off, that opens the avenue to run expect_identical(class(x), k) for the case outlined above. Or we could open it for expect_equal().

At Google, we've really aligned on expect_identical() being the default test, which is why we still lint for expect_identical() -- otherwise we wouldn't really catch anything to begin with. (There's another expect_identical_linter() forthcoming that blocks expect_equal() besides exceptional cases)

[jimhester]

What is wrong with expect_s3_class(data.table::data.table(), c("data.table", "data.frame")) it doesn't actually check for the exact order, it is just passed to inherits(), which just checks for membership. e.g. both these pass.

library(testthat)
expect_s3_class(data.table::data.table(), c("data.frame", "data.table"))
expect_s3_class(data.table::data.table(), c("data.table", "data.frame"))

^{Created on 2022-03-15 by the reprex package (v2.0.1)}

[jimhester]

Oh actually I forgot how inherits works, it actually just returns true if any of the input classes are found, not all of them, so separate calls to expect_s3_class() are probably what you would want anyway.

[MichaelChirico]

Right, e.g. rlang::inherits_any() is just a wrapper for inherits()... in priniciple the inheritance order could matter, and we might want to test for that. I'm not sure how often that's an issue though. It didn't come up at all in our internal packages, for example.

[MichaelChirico]

Another BTW, I have been running the new linters on lintr itself -- should I file the fixes in the same PR, or as a follow-up? This linter found some matches for us:

lintr/tests/testthat/test-methods.R:83:3: warning: expect_s3_class(x, k) is better than expect_true(is.<k>(x)) or expect_true(inherits(x, k)). Note also expect_s4_class() available for testing S4 objects.
  expect_true(is.data.frame(no_lint_summary))
  ^~~~~~~~~~~
lintr/tests/testthat/test-methods.R:92:3: warning: expect_s3_class(x, k) is better than expect_true(is.<k>(x)) or expect_true(inherits(x, k)). Note also expect_s4_class() available for testing S4 objects.
  expect_true(is.data.frame(has_lint_summary))
  ^~~~~~~~~~~

[AshesITR]

I think minor de-lints are fine if they don't make the PR too large.

[AshesITR]

false positive lint caused by glue: local variable ‘is_class_call’ assigned but may not be used.

[AshesITR]

Almost gtg