-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit b038e8f.
Merge branch 'main' into S7_objects # Conflicts: # R/aes.R # R/plot-construction.R # R/plot.R # R/theme.R
Merge branch 'main' into S7_elements # Conflicts: # DESCRIPTION
Merge branch 'main' into S7_objects # Conflicts: # DESCRIPTION
|
||
#' @export | ||
print.uneval <- function(x, ...) { | ||
`print.ggplot2::mapping` <- function(x, ...) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know the timeline for resolving this?
Merge branch 'S7_objects' into S7_elements # Conflicts: # NAMESPACE # R/plot-construction.R # R/theme.R # R/zzz.R # man/ggplot_add.Rd # tests/testthat/test-theme.R
S7 elements
#6355 is now merged in via teunbrand#16 |
I'm slightly annoyed that we have a |
I'm open to suggestions, but can also live a long and happy life with the discrepancy |
Alright let's not worry about it too much then :) |
This PR aims to fix a large part of #6352.
Briefly it implements the following S7 classes:
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.