-
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 S3 methods for fixed designs #482
base: main
Are you sure you want to change the base?
Conversation
Counter proposal from @yihui: The method-specific S3 functions are mainly defining strings like Main advantage compared to my current proposal in this PR: Much less boiler plate code. We'd have a single Main disadvantage compared to my current proposal in this PR: The default values are less transparent. When defined as the default arguments to a method-specific S3 function, then the default values are documented in the man page. When specified as attributes, a user would have to find the default values via the source code or by querying the hidden attributes. My thoughts:
|
The current approach brings too much boilerplate code, which makes me feel like shooting a mosquito with a canon (S3), but you may be right that S3 could start to show real value as we work on other functions. We'll see. I'm not sure if users will ask the question "where did the default title/footnote come from?". The scenario may be more like: users generate a table or summary, see the automagic default title/footnote, and either accept them with no questions asked, or provide their own titles/footnotes via arguments. The origin of the defaults is a implementation detail that's not worth their attention. Transparency can be helpful, but I don't know how valuable it is to show these lengthy strings on the help pages. I think the path will clearer as you move deeper into the forest. I can't make a clear judgment at the moment. |
392e05e
to
90e2888
Compare
This is a proof-of-concept PR related to our discussion of the classes in #457
Instead of using conditional statements via
if
/else
/switch
to mimic OOP, I convertedsummary.fixed_design()
,as_gt.fixed_design()
, andas_rtf.fixed_design()
to instead use hierarchical S3 classes to dispatch the correct function for the given "method", eg "ahr".I'll explain how this works using
as_gt()
. When callingas_gt()
on the result of asummary()
of a design object, it will first call its method-specificas_gt()
. In the case of data produced byfixed_design_ahr()
, this will beas_gt.design_fixed_ahr_summary()
. This function sets the method-specific title and footnote, and then passes this information viaNextMethod()
to the more generalas_gt.design_fixed_summary()
, which contains the actual {gt} code to produce the table.summary()
andas_rtf()
work in similar ways, defining the method-specific data and then passing this to the generic function used by all methods.Here is an example with ahr:
Pros:
Cons:
as_gt()
, we'd have to add it multiple placesOther notes:
to_integer()
and thegs_design
S3 generics. I started with thefixed_design
S3 generics since they were the simplest.as_rtf()
so that the superscript"{a}"
is always added, even for custom titles and footnotes. In the current version, the superscript is lost when providing a custom title or footnote. I think my update is an improvement, and thus propose we update the snapshot testssummary()
. It doesn't make sense for both the input and output object ofsummary()
to have the same classes when they are fundamentally different (ie we have designedas_gt()
to only work on the output fromsummary()
, and it shouldn't be applied to the output fromfixed_design_ahr()
)