Skip to content

Reimplement classes in S7 #6364

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

Merged
merged 119 commits into from
Jun 11, 2025
Merged

Reimplement classes in S7 #6364

merged 119 commits into from
Jun 11, 2025

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Mar 17, 2025

This PR aims to fix a large part of #6352.

Briefly it implements the following S7 classes:

  • class_ggplot
  • class_ggplot_built
  • class_mapping
  • class_labels
  • class_theme

I've put in the usual extractors and replacers ([, [[, $ and their <- methods) for the ggplot/ggplot_built classes for backwards compatibility. The code internally now uses @ though, so that we can deprecate the usual extractors/replacers later.

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jun 5, 2025

I think it is necessary to make ggplot() a S3 generic again. I have the following reasons:

  1. Extension have S3 methods for the S3 generic.
    • You cannot write S3 methods for an S7 generic.
    • There are too many extensions to force them all to use S7 right now.
  2. Unlike S3 ggplot_build(), which we plan to replace with S7 build_ggplot(), the ggplot() function is much more user-facing. I don't think we want to phase-out ggplot() for other function names.
  3. If people want to use S7 methods, they can register them for the S3 generic without any issues.

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jun 10, 2025

This is my take on the revdepcheck as it stands.

Of the 648 packages that I've annotated:

  • 91 fail because they have tests that pertain to classes in ggplot2. Most of these tests are of poor quality, like testing the class() vector itself instead of using inherits() or expect_s3_class() or is_ggplot(). This is very much an S7-transition related failure.
  • 69 fail due to errors in vignettes rather than tests or examples. I've not looked into these in much detail.
  • 64 are due to plotly, which is not due to the S7-transition. Relevant changes were addressed here:
    Updates in support of ggplot2 v4.0.0 plotly/plotly.R#2442
  • 63 are due to GGally, which was due to S7-transition, but I've sent out a PR:
    Compatibility with ggplot2 4.0.0 ggobi/ggally#528
  • 43 failures are unknown, which just means that I don't understand them fully
  • 32 packages do invalid stuff with labels, like labs(y = element_blank()). This is related to the S7 transition as we're now more strict about such things.
  • 31 packages test for labels that are no longer there. This is not related to the S7 transition.
  • 25 packages are due to patchwork's & operator that doesn't quite work well out of the box.
  • 23 packages have namespace conflicts, like sjPlot::set_theme() or e1071::element(). Some of them are due to S7 transition, some of them aren't. In any case, these packages should probably be more careful with their imports to avoid conflicts.
  • 23 packages are probably false positives that get picked up because they emit warnings during package building.
  • 15 packages set properties incorrectly, like element_text(family = TRUE, colour = 12). Technically using an integer for colour should index the base graphics palette, but I don't think this should work. This is due to the S7 transition because we are more strict.
  • 12 packages use functions that we've fully deprecated now. This isn't due to the S7 transition.
  • 11 packages use internals that have changed, which isn't related to the transition.
  • 10 packages construct incorrect margins, like margin(c(1, 2, 3, 4)) instead of margin(1, 2, 3, 4). This is an S7-transition thing, as we're now more strict about this.
  • 8 packages have na.value that cannot be combined with palette values, which isn't related to S7 transition.
  • 5 packages are due to qqplotr, haven't dig too deeply yet, but I'll send a PR.
  • 5 packages fail because they manually construct structure(..., class = "uneval") instead of aes(). This is due to S7 transition, because we now have a <mapping> class.
  • 5 packages fail because their test don't expect plot$layers to be named. This isn't related to the S7-transition.
  • 5 packages fail because they use ggtern.
  • 4 packages don't have a tibble import, whereas they should.
  • 4 packages use ggplot2 in very non-standard ways
  • 4 packages expect to be able to as.list(<ggplot>), whereas they couldn't at the time of the check.
  • 4 packages fail due to ggiraph, for which I've made a PR:
    Compatibility with ggplot2 4.0.0 davidgohel/ggiraph#319
  • 4 packages use aes(label = <expression>), which they shouldn't.
  • 4 packages had problems that I couldn't locally reproduce.
  • 4 packages were in trouble to to ggtext.
  • 3 packages do ifelse(test, <plot>, ...) istead of if (test) {<plot>} else {...}. This worked because ggplot objects were lists, i.e. vectors, but they aren't anymore.
  • 3 packages due to ggparty.
  • 3 packages due to gganimate.
  • 3 packages improperly access geom defaults, they should use get_geom_defaults().
  • Various other problems that are infrequent or specific to single packages that I won't all discuss here.

@cpsievert
Copy link
Contributor

Please bump the version to something like 3.5.2.9001, so we can switch logic depending on the version

@teunbrand
Copy link
Collaborator Author

I don't see how we can further minimise the impact of this PR without abandoning the S7 transition altogether. I'll merge this into main so I can send out some PRs and instruct to check with the dev version, which is easier than check with a PR.

@teunbrand teunbrand merged commit d57ce4c into tidyverse:main Jun 11, 2025
13 checks passed
@teunbrand teunbrand deleted the S7_objects branch June 11, 2025 06:59
@teunbrand teunbrand mentioned this pull request Jun 11, 2025
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.

4 participants