Skip to content

Create chk_s7_class() - #308

Open
joethorley wants to merge 4 commits into
mainfrom
254-chk-s7-class
Open

joethorley wants to merge 4 commits into
mainfrom
254-chk-s7-class

Conversation

@joethorley

Copy link
Copy Markdown
Member

Closes #254

Stacked on #306 — base branch is 253-chk-r6-class, because this builds directly on the object_type() helper introduced there and would otherwise conflict in _pkgdown.yml and the vignette's Class Checkers table. Review/merge #306 first; the diff shown here is only the S7 commit.

Adds chk_s7_class() and vld_s7_class():

vld_s7_class <- function(x, class) {
  inherits(x, "S7_object") && inherits(x, class)
}

The R >= 4.2 note on the issue does not apply

The issue says to wait for R >= 4.2 so that inherits(obj, <S7 class>) works. That constraint applies only to passing the S7 class generator object as class. chk's class parameter is documented as "A character vector specifying the possible class values", and with character names plain inherits() works on any supported R:

class(x):                     mypkg::ChildS7class, mypkg::ParentS7class, S7_object
inherits(x, "mypkg::ChildS7class"):   TRUE
inherits(x, "mypkg::ParentS7class"):  TRUE     # S7 puts the whole chain in class()

So no Depends bump and no new dependency. S7 stays in Suggests, 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 foo is matched by 'foo::ClassName', not 'ClassName'. This is consistent with every other chk class checker — they all match against class(x) — but it is a papercut, so it is documented in @details, shown in the examples, and pinned by a test:

expect_identical(class(x), c("mypkg::exampleS7class", "S7_object"))
expect_false(vld_s7_class(x, "exampleS7class"))

I found this the hard way: my first test run under devtools::load_all() passed, then the full suite failed because inside the package context S7::new_class("exampleS7class") becomes chk::exampleS7class. All tests now pass package = explicitly so they are deterministic regardless of context.

If you would rather vld_s7_class() also accept an S7 class object for class and unwrap the name itself, that is a reasonable follow-up, but it needs conditional requireNamespace("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")) is c("S7_class", "S7_object"). Unlike R6 generators, which are not R6 objects at all, an S7 generator therefore passes vld_s7_class(generator, "S7_class") — while correctly failing vld_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 an S7 branch, so it reports 'S7' where it previously reported 'S3':

} else if (inherits(x, "S7_object")) {
  "S7"
}

This changes the object type named in the failure messages of chk_s3_class(), chk_s4_class() and chk_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 it chk_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 of vld_s3_class() — an S7 object still passes chk_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.R plus 2 for the object_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

joethorley and others added 4 commits August 30, 2026 13:10
Checks inherits from R6 class using

```r
inherits(x, "R6") && inherits(x, class)
```

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
@joethorley

Copy link
Copy Markdown
Member Author

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. object_type() gaining an S7 branch makes chk_s3_class(), chk_s4_class() and chk_r6_class() report "not S7 class ..." where they previously said "not S3 class ...".

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.

Base automatically changed from 253-chk-r6-class to main August 31, 2026 15:19

@StefanoMezzini StefanoMezzini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@joethorley

Copy link
Copy Markdown
Member Author

You might be right but I don't want to risk it - it was very time consuming updating dependencies

@StefanoMezzini

Copy link
Copy Markdown
Member

Sounds good. Merging in the conflicts with the main branch might just get very complicated

@joethorley
joethorley marked this pull request as ready for review September 8, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add checks for S7 class

2 participants