Skip to content
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

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

Closed
nanxstats opened this issue May 3, 2024 · 5 comments · Fixed by #247
Closed

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

nanxstats opened this issue May 3, 2024 · 5 comments · Fixed by #247
Assignees
Labels
best-practice Enhance programming best practices

Comments

@nanxstats
Copy link
Collaborator

nanxstats commented May 3, 2024

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().

@nanxstats nanxstats added the best-practice Enhance programming best practices label May 3, 2024
@jdblischak
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
yihui/xfun@39c8802
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
Copy link
Contributor

yihui commented May 3, 2024

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
Copy link
Collaborator

jdblischak commented May 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
best-practice Enhance programming best practices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants