Tweaking as_gt() #449
Tweaking as_gt() #449
Conversation
… original code, which is harmless but also unnecessary)
…ing their values in the last unnamed argument, which will be treated as the default
… attributes() and then getting one from the list
… worth using the pipe
… to `switch(method)`; also factor out the phrase 'Bound summary'
…different element via `switch()` for the four methods
…h more readable; also replace `select(all_of(vars))` with `[, vars]`
… them in `content`, `location`, and `attr` for all four methods; also use `if` instead of `ifelse()`, so that we don't need to filter out NA values in the end note that this introduces whitespace changes in snapshot tests, which are harmless (these spaces are meaningless in LaTeX and HTML)
…ivalent to simply `for (i in seq_along(x))`; the two `if` statements are unnecessary, because if `x` is `NULL` or of length 0, the `for` loop will be skipped also factor out the footnote location element to the object `loc`, so we don't have to repeat `x %>% gt::tab_footnote()` five times
R/ahr_blinded.R
Outdated
| event = sum(event), | ||
| ahr = exp(-theta), | ||
| theta = theta, | ||
| info0 = sum(event) * (1 - q_e) * q_e | ||
| ) | ||
| return(ans) |
There was a problem hiding this comment.
Personally I like the explicit return even though it is optional. And checking git blame, I'm pretty sure they existed before I started editing the code base. I'm happy to go along with the consensus though
There was a problem hiding this comment.
I don't have a strong opinion here and can definitely use an explicit return(). Personally I use return() only when I need to return early.
There was a problem hiding this comment.
@LittleBeannie Do you have a preference here? As I said, I'm totally fine with reverting to the original style ans <- ...; return(ans), even though both the assignment and the explicit return() look unnecessary to me.
There was a problem hiding this comment.
Thank you for verifying, @yihui ! I personally prefer the explicit return, as it provides a clearer message for non-advanced R programmers.
There was a problem hiding this comment.
Got it. I've restored return() in 90352a0.
There was a problem hiding this comment.
Thank you, @yihui, for tweaking the long as_gt function! I only have 2 minor comments, regarding the error message. Could you please take a look?
| } | ||
| if (!is.vector(intervals, mode = "numeric") || min(intervals) <= 0) { | ||
| stop("ahr_blinded: intervals must be a vector of positive numbers.") |
There was a problem hiding this comment.
Hi Yihui, I have a question regarding the error message output. Typically, our practice in gsDesign2 is to print the function name at the beginning of the error message. This consideration arises from the possibility that the ahr_blinded function could potentially be invoked within another function. So... I am wondering if we should keep the ahr_blinded here?
There was a problem hiding this comment.
Good question. I deleted the function name in the error message because I thought it would be redundant.
f = function() foo()
foo = function() {
stop('No!')
}
f()
#> Error in foo() : No!foo = function() {
stop('foo: No!')
}
f()
#> Error in foo() : foo: No!This relates to a comment I made after #448 (comment) That is, the function name will be missing if you do.call the function.
foo = function() {
stop('No!')
}
f = function() do.call(foo, list())
f()
#> Error in (function () : No!I think this case is relatively rare (so far we have used do.call() mostly with rbind/cbind in this package). If it does happen, traceback() can be helpful to know the chain of function calls.
That said, again I don't have a strong opinion here. In general, my principle is that I'll follow the preference of the project maintainer (i.e. you in this case).
There was a problem hiding this comment.
Thank you for the clear explanation; I understand your intention. I am open to this. If we choose to remove the function name at the beginning of the error message, we should review the entire gsDesign2 package, as the previous practice of gsDesign2 is to include the function name right at the start (maybe a new issue and PR).
There was a problem hiding this comment.
Okay, as I'm going through the whole codebase, I'll do this along the way.
There was a problem hiding this comment.
Thanks, @yihui! This is not urgent and thanks again for reviewing the codes!
There was a problem hiding this comment.
I searched the repository for how ahr_blinded() is used. From the package code, examples, and tests, ahr_blinded() is always called directly. In other words, the function itself is never passed as an argument to another function. Thus the fact that an error comes from ahr_blinded() should be clear from the default error message, as @yihui noted above.
To demonstrate, I added stop("there was an error") to ahr_blinded() and generated the error message below:
devtools::load_all()
ahr_blinded(
surv = survival::Surv(
time = simtrial::ex2_delayed_effect$month,
event = simtrial::ex2_delayed_effect$evntd
),
intervals = c(4, 100),
hr = c(1, .55),
ratio = 1
)
## Error in ahr_blinded(surv = survival::Surv(time = simtrial::ex2_delayed_effect$month, :
## there was an error| x <- x %>% dplyr::select(dplyr::all_of(display_columns)) | ||
| } | ||
| if (!all(display_columns %in% names(x))) stop( | ||
| "not all variable names in 'display_columns' are in the summary_bound object!" |
There was a problem hiding this comment.
Echo with my previous comment, shall we add as_gt at the beginning of the error message?
There was a problem hiding this comment.
Overall this is looking great. Please see the one switch() statement that needs adjusted.
A bigger picture question for @LittleBeannie and @nanxstats : are we still planning on occasionally running {styler} on the code in {gsDesign2}? The default {styler} format is significantly different than Yihui's preferred style. Below is an example git diff after running {styler} on this PR:
I'm looking at the R scripts by alphabetical order, and reduced ~180 lines of code in
as_gt(). Again, the changes may be easier to understand if you read the commits one by one.In some places, I'm using my personal style (e.g., for line breaks and indentation), and happy to follow the style that other colleagues are comfortable with. In general, I tend to make the code a little compact so I can scroll less when reading the code.
I also realized
as_rtf()'s source code is very similar toas_gt(), and perhaps can cut out more lines there lines after factoring out the common parts.