Skip to content

Streamline S7 parts #6546

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
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Jul 7, 2025

This PR amends #6364.

There are two workarounds we've used to make S7 work that, after some consideration, may not be necessary.

  1. To work around S7::method() returning a function in the namespace that then clashed with S3 functions, we used function_name.ggplot2::ggplot and its ilk. Since then we learned of using local() to avoid this problem, and it is arguably a better solution than hacking away at S3 methods. Except for replacement functions, this seems to work great.
  2. We introduced 4 S7 'adaptor generics' to facilitate transition between S3 and S7 as we cannot transfer the S3 methods written by extension packages to S7 generics. However, I've since learned that it is S7's intent to have S3 fallback for S7 generics in the future. At that point, the S7 adaptor generics would become vestigial. To prevent having to instigate a future deprecation cycle, I think it is best to keep these S3 generics with S7 methods for now. It concerns the following pairs:
    • build_ggplot() for ggplot_build()
    • update_ggplot() for ggplot_add() This uses double dispatch that cannot be S3.
    • draw_element() for element_grob()
    • gtable_ggplot() for ggplot_gtable()

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jul 7, 2025

Had to do a small consession because I already used build_ggplot() in rstudio/thematic#156 which is already on CRAN.

@teunbrand teunbrand added this to the ggplot2 4.0.0 milestone Jul 7, 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.

1 participant