Redirect as_gt() to gsDesign2 when it is masked by simtrial::as_gt()

[jdblischak]

Closes #284

cc: @yihui

Here is my proposed solution to fix the masking of the generic gsDesign2::as_gt() by the generic simtrial::as_gt():

# before
library("gsDesign2")
library("simtrial")
## Attaching package: ‘simtrial’
##
## The following object is masked from ‘package:gsDesign2’:
##
##     as_gt
##

gs_design_ahr() |> summary() |> as_gt()
## Error in UseMethod("as_gt", x) :
##   no applicable method for 'as_gt' applied to an object of class "c('non_binding', 'ahr', 'gs_design', 'grouped_df', 'tbl_df', 'tbl', 'data.frame')"
gs_design_ahr() |> summary() |> gsDesign2::as_gt()

# after
library("gsDesign2")
library("simtrial")
## Attaching package: ‘simtrial’
##
## The following object is masked from ‘package:gsDesign2’:
##
##     as_gt
##

# works without gsDesign2:: prefix
gs_design_ahr() |> summary() |> as_gt()

[jdblischak]

Surprisingly R CMD check --as-cran does not complain about the use of :::. I thought I was going to have to resort to trickery such as utils::getFromNamespace(). I am worried though that this might get flagged by one of the extrachecks (eg not documenting the return value). Not sure though since this is an S3 method and not a standalone function 🤞

[yihui]

We probably should take one step back and rethink why we need the S3 generic as_gt() in the first place. If the answer is pretty printing of results, we may consider extending base::print() instead just like we did with base::summary(). The advantages are: 1) print()ing is implicit in R, so users don't need to call it explicitly unless they want to change default argument values; 2) it exists in base R, so we don't need to define the generic but only the methods; 3) a single generic print() won't create conflicts as two separate generics do (as_gt() in both packages).

If we want pretty printing in R Markdown/Quarto/litedown, we'll need to define S3 methods for knitr::knit_print and xfun::record_print. In these methods, we can automatically determine whether we want to print the table as gt or rtf.

I don't understand the purpose of as_gt() well, but from my experience, two functions with the same name in two separate packages are doomed to create trouble for both developers and users. In my eyes, dplyr::filter() was the worst mistake in Tidyverse, and unfortunately, there's no way to completely put the worms back to the can once the can is opened (ad hoc measures exist but the confusion will last forever). That's why I'm so glad that we haven't released simtrial::as_gt to CRAN yet.

If we really need as_gt(), we'd better define it once in a certain "base" package (such as msdtools mentioned by Nan), and let other packages extend it.

Surprisingly R CMD check --as-cran does not complain about the use of :::.

I take the credit shamelessly. Back in 2013 when I was still bold and not bald, I had a long debate with CRAN maintainers, and eventually they agreed to allow ::: for the case of packages with the same maintainer. That is, if two packages have the same maintainer, the two packages can use ::: to get unexported functions from each other. Yujie is the maintainer of both simtrial and gsDesign2, so we are free to steal internal objects from one package to another.

[jdblischak]

we may consider extending base::print() instead just like we did with base::summary()

I just remembered another complication with using base::print(). I had actually already tried to create S3 methods for print() to solve a different problem in {gsDesign2}. My attempt was stalled because the input and output to summary() have the same classes (Merck/gsDesign2#392 (comment))

[LittleBeannie]

Thanks, @jdblischak ! A quick question: shall we also update gsDesign2 similarly?

[jdblischak]

shall we also update gsDesign2 similarly?

@LittleBeannie Yes. Already done in Merck/gsDesign2#467. Please add both of these PRs to the agenda to discuss at Friday's group meeting. Thanks!

[LittleBeannie]

shall we also update gsDesign2 similarly?

@LittleBeannie Yes. Already done in Merck/gsDesign2#467. Please add both of these PRs to the agenda to discuss at Friday's group meeting. Thanks!

That's great, thank you! Will do.

[yihui]

Regarding :::, actually I forgot to ask whether it would work by calling gsDesign::as_gt() instead of the specific method:

Suggested change

	gsDesign2:::as_gt.fixed_design(x, ...)
	gsDesign2::as_gt(x, ...)

Of course, it will be better if we could avoid using :::.

[jdblischak]

Unfortunately it doesn't work with only ::

library("gsDesign2")
library("simtrial")
## Attaching package: ‘simtrial’
##
## The following object is masked from ‘package:gsDesign2’:
##
##     as_gt
##

gs_design_ahr() |> summary() |> as_gt()
## Error: 'as_gt.gs_design' is not an exported object from 'namespace:gsDesign2'

[yihui]

Interesting. Thanks for confirming!