Skip to content

Revdep: checking for classes #6498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
teunbrand opened this issue Jun 11, 2025 · 12 comments
Open

Revdep: checking for classes #6498

teunbrand opened this issue Jun 11, 2025 · 12 comments
Labels
breaking change ☠️ API change likely to affect existing code internals 🔎

Comments

@teunbrand
Copy link
Collaborator

teunbrand commented Jun 11, 2025

Problem

This issue emerged from a recent revdepcheck (de2715d), and affects an estimated 91 reverse dependencies.
The issue is that tests based on ggplot2's classes fail because we transitioned to S7 classes.

It includes tests for the class itself

expect_equal(class(p), c("gg", "ggplot"))

But also tests that relate to class structure like so:

expect_length(p, 11)
expect_type(p, "list")

The following case works for now, but we might remove the manual "ggplot" S3 class in the future. It is not the recommended way of testing.

expect_s3_class(p, "ggplot")`

This pattern may hold for other classes, like mapping, labels, margin or elements too.

Solution

There are several possibilities for adjusting the test.

  • The preferred way is to use expect_true(is_ggplot(p)). The is_ggplot() function is internally consistent regardless of whether the ggplot object is the old or new class.
  • Alternatively, you can use expect_true(inherits(p, c("ggplot", "ggplot2::ggplot"))), which requires a match to one of the two classes.
  • Lastly, you can use expect_s7_class(p, class_ggplot), but that will only be stable after we've released the new version. It is not suitable for backward compatibility.

Unfortunately, this is not something that is straightforward to fix in ggplot2, so this probably will have to be updated in the reverse dependencies.

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jun 11, 2025

Informed by email, as there is no linked GitHub page in the DESCRIPTION files:

  • ASRgenomics
  • bdsm
  • benchr
  • calibmsm (received response)
  • carbonr (out of office)
  • CausalImpact
  • cmcR
  • conquestR (received response)
  • fChange
  • gfoRmulaICE (bounced)
  • ggbump (received response)
  • ggstream (received response)
  • miRetrieve
  • mshap
  • mxnorm
  • NiLeDAM (received response)
  • rbioacc (Revdep: checking for classes #6498 (comment))
  • rifreg
  • rPBK
  • RVenn
  • Rwtss
  • SOMbrero (received response)
  • tidyEdSurvey

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jun 11, 2025

Mistakingly classified a related problem for this exact problem:

  • biclustermd
  • canvasXpress
  • ggblend
  • listdown
  • ggparalllel
  • scplot
  • synthpop

@teunbrand
Copy link
Collaborator Author

Not informed:

  • clifro (archived)

@virgile-baudrot
Copy link

Hi @teunbrand
thank for the e-mail. i'll try to fix it for rbioacc within 2 weeks, issue rbioacc. I'll give you feedback here, when it's done and resubmit to CRAN.
Best

@billdenney
Copy link
Contributor

Thanks for the hard work to improve ggplot2!

I'm working on the fix to nlmixr2plot, and the class check changes are straight-forward.

We have a method for addition that is now broken. Generally it looks like our_object + ggplot2::xlab("new label") and that returns NULL now. Do you have a pointer for how to make addition with the new S7 ggplot2 object work where we want to add a ggplot2 object to our object?

@teunbrand
Copy link
Collaborator Author

We have a method for addition that is now broken

I'm also struggling with that myself, I've made a separate issue to discuss: #6504

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ☠️ API change likely to affect existing code internals 🔎
Projects
None yet
Development

No branches or pull requests

5 participants