Create chk_s7_class() - #308
joethorley wants to merge 4 commits into
Conversation
Removes the S4/R6/S3 object-type expression triplicated across `chk_s3_class()`, `chk_s4_class()` and `chk_r6_class()`. No change to error messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Checks inherits from S7 class using ```r inherits(x, "S7_object") && inherits(x, class) ``` `object_type()` now reports S7 objects as `'S7'` rather than `'S3'`, which changes the object type named in the error messages of `chk_s3_class()`, `chk_s4_class()` and `chk_r6_class()` when they are given an S7 object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Holding this as a draft until after the next CRAN release. Not because anything is wrong with it, but because it is the only one of the four in flight that changes existing error message text. That is precisely the failure mode that produced 5 new problems in the 0.11.0 revdep run (mcmcr, nlist, readwritesqlite, sims, term), all of them downstream tests asserting on chk's exact wording. The exposure here is narrow, since it only shows up when an S7 object is passed to one of those checkers and the check fails, so quite possibly no revdep touches it. But narrow is not zero, and it is not worth spending revdep goodwill on a message refinement in the same release as the new checkers. #306, #309 and #310 are all message-neutral and go in first. This gets rebased and readied for the release after. |
StefanoMezzini
left a comment
There was a problem hiding this comment.
I think we could merge this in. Aren't all the uses of {S7} skippable and behind checks? We could drop the {S7} dependency in a separate PR once the class becomes part of R.
|
You might be right but I don't want to risk it - it was very time consuming updating dependencies |
|
Sounds good. Merging in the conflicts with the main branch might just get very complicated |
Closes #254
Stacked on #306 — base branch is
253-chk-r6-class, because this builds directly on theobject_type()helper introduced there and would otherwise conflict in_pkgdown.ymland the vignette's Class Checkers table. Review/merge #306 first; the diff shown here is only the S7 commit.Adds
chk_s7_class()andvld_s7_class():The
R >= 4.2note on the issue does not applyThe issue says to wait for
R >= 4.2so thatinherits(obj, <S7 class>)works. That constraint applies only to passing the S7 class generator object asclass. chk'sclassparameter is documented as "A character vector specifying the possible class values", and with character names plaininherits()works on any supported R:So no
Dependsbump and no new dependency.S7stays inSuggests, used only by tests and examples.Two behaviours worth your attention
1. Class names are package qualified. S7 qualifies class names with the defining package, so a class defined in package
foois matched by'foo::ClassName', not'ClassName'. This is consistent with every other chk class checker — they all match againstclass(x)— but it is a papercut, so it is documented in@details, shown in the examples, and pinned by a test:I found this the hard way: my first test run under
devtools::load_all()passed, then the full suite failed because inside the package contextS7::new_class("exampleS7class")becomeschk::exampleS7class. All tests now passpackage =explicitly so they are deterministic regardless of context.If you would rather
vld_s7_class()also accept an S7 class object forclassand unwrap the name itself, that is a reasonable follow-up, but it needs conditionalrequireNamespace("S7")code in package code, which is why I did not do it here.2. A generator is itself an S7 object. S7 is self-describing, so
class(S7::new_class("Foo"))isc("S7_class", "S7_object"). Unlike R6 generators, which are not R6 objects at all, an S7 generator therefore passesvld_s7_class(generator, "S7_class")— while correctly failingvld_s7_class(generator, "mypkg::Foo"), since it is not an instance of the class it generates.I chose not to special-case this. Excluding
'S7_class'would make the check lie about an object that genuinely is an S7 object of that class. It is one&& !inherits(x, "S7_class")away if you disagree.Error message change
object_type()gains anS7branch, so it reports'S7'where it previously reported'S3':This changes the object type named in the failure messages of
chk_s3_class(),chk_s4_class()andchk_r6_class()when they are handed an S7 object — e.g. "not S7 classes 'mypkg::Foo' and 'S7_object'" instead of "not S3 classes ...". Without itchk_s7_class()would report S7 objects as S3, which reads as a bug. No existing test asserted a message for an S7 input, so nothing needed updating.Note this sits slightly awkwardly with
chk_s3_class()'s@details("base objects and S7 classes are considered S3 objects"), which remains true ofvld_s3_class()— an S7 object still passeschk_s3_class(). Only the failure message wording changes. Say the word if you would rather the message keep saying S3.Verification
Written test-first; 20 new assertions in
tests/testthat/test-chk-s7-class.Rplus 2 for theobject_type()S7 branch, confirmed failing before implementation.devtools::test():[ FAIL 0 | WARN 0 | SKIP 0 | PASS 1339 ].devtools::check(): 0 errors, 0 warnings, 0 notes.🤖 Generated with Claude Code