-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor methods by passing table metadata as attributes #497
base: main
Are you sure you want to change the base?
Conversation
This looks clear to me. I apologize for the complexity - if we could have written this in classical OOP, things would have been much more straightforward. This is one of the few things where I think Python gets it right but base R didn't. |
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.
This is fantastic. I absolutely love it! Previously I felt like we invented a homemade and awkward "S2" paradigm (via switch(class(x), ...)
) inside S3.
I had trouble finding a source that explicitly said it was fine to omit the class x <- list()
class(x)
#> [1] "list"
is.list(x)
#> [1] TRUE
inherits(x, "list")
#> [1] TRUE
print(x)
#> list()
class(x) <- "custom"
class(x)
#> [1] "custom"
is.list(x)
#> [1] TRUE
inherits(x, "list")
#> [1] FALSE
print(x)
#> list()
#> attr(,"class")
#> [1] "custom"
class(x) <- c("custom", "list")
class(x)
#> [1] "custom" "list"
is.list(x)
#> [1] TRUE
inherits(x, "list")
#> [1] TRUE
print(x)
#> list()
#> attr(,"class")
#> [1] "custom" "list" |
I think the following might be the canonical approaches of creating and assigning S3 classes to a list, that also shows they are equivalent: x <- list()
class(x) <- "my_class"
is.list(x)
#> [1] TRUE
inherits(x, "list")
#> [1] FALSE x <- structure(list(), class = "my_class")
is.list(x)
#> [1] TRUE
inherits(x, "list")
#> [1] FALSE I'd do the entire thing because any intermediate results can generate some surprises. And yes, I feel assigning class |
Hi @jdblischak, I saw this PR is in progress. Is it ready for merge? |
No. It is still a Draft. I have updated the |
This is an alternative approach to #482 to refactor the classes/methods as discussed in #457
This PR refactors the
fixed_design_X()
functions to define their owndesign_display
,title
, andfootnote
as attributes, which are then used bysummary()
,as_gt()
, andas_rtf()
. This idea was proposed by @yihui, which I described in #482 (comment)By using the above strategy, I was able to make the following improvements to the classes/methods:
summary.fixed_design()
no longer adds a method class to its output object (eg"ahr"
) since this is no longer necessary. This was only used to constructed the table metadata, which is now already defined in the attributes of the object produced the respectivefixed_design_X()
functionsummary.fixed_design()
returns an object of class"fixed_design_summary"
to distinguish it from the output of afixed_design_X()
function (with class"fixed_design"
)Below is example code to test out the changes in this PR: