Big refactor to simplify S3 classes

[jdblischak]

Closes #457
Closes #497

This PR finalizes the proposal I introduced in #497. The long-term goal is to unblock Issues like #392, which will be easily solvable after this PR has been merged.

I overhauled the S3 classes according to the following principles:

• An object should only be assigned an S3 class if there is at least one corresponding method. If there is no corresponding method, pass the information as an attribute
• The same class should not be applied to different outputs (as is currently done where summary() returns the same class)

Some decisions I made that should be reviewed:

• Instead of assigning the method as a class for the group sequential functions, I added design to the returned list, just like the fixed designs do
• I didn't touch the gs_X_npe() functions. I don't fully understand why, but their structure is different than the others
• I updated gs_update_ahr() to add the attribute updated_design = TRUE instead of adding a class "updated_design" (following the principles above, there is no corresponding method). However, I'm not sure this is really necessary. Do we really need this information? I couldn't find anywhere in {gsDesign2} or {simtrial} where the class was queried for "updated_design"
• Instead of having the class "non_binding" which has no corresponding methods, I added an attribute binding that gets passed the input argument binding, and is thus TRUE or FALSE
• I left to_integer() mostly untouched. The engineer in me wanted to pull apart and refactor this as well (which I actually did in a different branch), but I felt the elegance of having an S3 to_integer() method for each design method didn't outweigh the improvements of removing the methods from the class hierarchy

devtools::load_all()
x <- gs_design_ahr()
x$design
## [1] "ahr"
class(x)
## [1] "gs_design"
attributes(x)
## $names
## [1] "design"      "input"       "enroll_rate" "fail_rate"   "bound"       "analysis"
## 
## $class
## [1] "gs_design"
## 
## $binding
## [1] FALSE
## 
## $uninteger_is_from
## [1] "gs_design_ahr"
## 
y <- to_integer(x)
y$design
## [1] "ahr"
class(x)
## [1] "gs_design"
attributes(x)
## $names
## [1] "design"      "input"       "enroll_rate" "fail_rate"   "bound"       "analysis"
## 
## $class
## [1] "gs_design"
## 
## $binding
## [1] FALSE
## 
## $uninteger_is_from
## [1] "gs_design_ahr"
## 
z <- summary(y)
class(z)
## [1] "gs_design_summary" "grouped_df"        "tbl_df"            "tbl"               "data.frame"
attributes(z)
## $class
## [1] "gs_design_summary" "grouped_df"        "tbl_df"            "tbl"               "data.frame"
## 
## $row.names
## [1] 1 2
## 
## $names
## [1] "Analysis"             "Bound"                "Z"                    "~HR at bound"
## [5] "Nominal p"            "Alternate hypothesis" "Null hypothesis"
## 
## $groups
## # A tibble: 1 × 2
##   Analysis                                                                          .rows
##   <chr>                                                                       <list<int>>
## 1 Analysis: 1 Time: 35.8 N: 478 Events: 292 AHR: 0.68 Information fraction: 1         [2]
## 
## $binding
## [1] FALSE
## 
## $design
## [1] "ahr"
## 
## $full_alpha
## [1] 0.025
## 
as_gt(z)

[yihui]

Just a minor feedback. I think this is all great! Thanks!

[yihui]

Previously we could disable the footnote via footnote = FALSE. Now we wouldn't if this if statement is removed.

[jdblischak]

Good catch! Fixed by c8609a1

[yihui]

I wonder if it's worth exposing the true defaults here, i.e.,

Suggested change

	as_gt.fixed_design_summary <- function(x, title = NULL, footnote = NULL, ...) {
	as_gt.fixed_design_summary <- function(x, title = attr(x, "title"), footnote = attr(x, "footnote"), ...) {

Two benefits are: 1) it'd be more transparent; 2) it'd make it possible to actually use NULL. The footnote = FALSE mentioned below was a hack that I resorted to because in the Shiny app I did need to disable the footnote. Although the natural choice was footnote = NULL, it just couldn't work because we override the NULL value by our defaults.

I don't have strong opinions on this, so if you prefer the current way, that will be fine to me.

[jdblischak]

I like your proposal, but I don't want to introduce any API changes in this already large PR. Thus for now I am following the documented interface:

gsDesign2/R/as_gt.R

Line 150 in 936ec20

#' the table. To disable footnotes, use `footnote = FALSE`.

But given that you were the one that introduced the workaround footnote = FALSE (#514), please feel free to send a follow-up PR to adjust this behavior.

[yihui]

Sounds good. I'll follow up.

[yihui]

I decided not to change this after looking at as_gt.gs_design_summary, in which the footnote logic was more complicated than that in as_gt.fixed_design_summary. Let's continue to use footnote = FALSE as the only way to disable footnotes.

[jdblischak]

Thanks for the review @yihui! I restored the behavior of as_gt(footnote = FALSE) and added some test cases, but I decided against changing the function interface in this PR

[jdblischak]

Good catch! Fixed by c8609a1

[jdblischak]

I like your proposal, but I don't want to introduce any API changes in this already large PR. Thus for now I am following the documented interface:

gsDesign2/R/as_gt.R

Line 150 in 936ec20

#' the table. To disable footnotes, use `footnote = FALSE`.

But given that you were the one that introduced the workaround footnote = FALSE (#514), please feel free to send a follow-up PR to adjust this behavior.