Merck / simtrial

Clinical trial simulation for time-to-event endpoints
https://merck.github.io/simtrial/
GNU General Public License v3.0
16 stars 8 forks source link

Replace `rlang::is_installed()` with a lightweight solution #246

Closed nanxstats closed 4 months ago

nanxstats commented 4 months ago

I used rlang::is_installed() and introduced rlang as a hard dependency in #245 to safeguard code examples, tests, and vignettes and get the package pass the noSuggests check.

It would be ideal if we can use a lightweight solution here and remove the dependency.

@jdblischak suggested requireNamespace(). @yihui also mentioned xfun::loadable().

jdblischak commented 4 months ago

I propose the following internal function:

is_installed <- function(package) requireNamespace(package, quietly = TRUE)

The only difference is that it returns its value invisibly

nanxstats commented 4 months ago

Sounds good. xfun::loadable() does a bit more by using suppressPackageStartupMessages(requireNamespace(pkg, quietly = TRUE)) and I wonder if suppressPackageStartupMessages() handles some particular edge cases.

jdblischak commented 4 months ago

I wonder if suppressPackageStartupMessages() handles some particular edge cases.

Definitely an edge case. I confirmed locally that requireNamespace() can suppress typical package startup messages. But apparently if a package has S3 methods that overwrite existing ones, then these messages are reported.

https://github.com/yihui/xfun/blame/9183dcc56d54f6ab1ef0d33c2eb86dfe8d0346b6/R/packages.R#L87 https://github.com/yihui/xfun/commit/39c8802701be9f10866bbd294d87be35b7abbae4 https://stat.ethz.ch/pipermail/r-devel/2019-May/077774.html

Though I'm not sure we want to suppress these. Let's say a user runs example("get_cut_date_by_event"). If they have {dplyr} installed, then the code will execute. If any S3 methods in {dplyr} overwrite existing S3 methods in their current session, they probably want to know about that. Previously they would have been notified by the subsequent library("dplyr"), but presumably not any more since the package was already loaded by requireNamespace().

Thoughts?

yihui commented 4 months ago

I propose the following internal function:

is_installed <- function(package) requireNamespace(package, quietly = TRUE)

The only difference is that it returns its value invisibly

I think that's good enough. No need to consider package startup messages, since this function is currently used only for R CMD check purpose.

jdblischak commented 4 months ago

The only difference is that it returns its value invisibly

I realized there is another key difference between rlang::is_installed() and base::requireNamespace(). The former accepts a vector as input, but the latter only checks the first listed package (and silently ignores the remaining packages).

rlang::is_installed(c("stats", "not-a-valid-package-name"))
## [1] FALSE

head(requireNamespace, 3)
##
## 1 function (package, ..., quietly = FALSE)
## 2 {
## 3     package <- as.character(package)[[1L]]

(requireNamespace(c("stats", "not-a-valid-package-name")))
## [1] TRUE