FredHutch / VISCtemplates

Tools for writing reproducible reports at VISC
Other
4 stars 2 forks source link

Respect existing repos option #163

Closed slager closed 2 weeks ago

slager commented 3 weeks ago

By default and when otherwise configured properly, R should not require specifying repos in install.packages() because getOption('repos') is used automatically.

The old behavior was to hardcode http://cran.us.r-project.org. This can be an annoyance. For example, it's common on Linux to set up custom repos in .Rprofile to install binary packages from Posit Public Package Manager, to greatly reduce install time for packages. During some VISCtemplates testing that called install_load_cran_packages(), packages got installed from source instead due to the hardcode, which took a lot longer.

The new behavior is to respect the existing repos option to follow best practices.

I also tweaked the way it looks for installed packages to avoid a warning when the package is not found, and have it returning invisibly instead of printing out the list of packages installed.

kelliemac commented 3 weeks ago

This looks good to me. My only comment is that the function name install_load_cran_packages is not really accurate anymore if we are not forcing packages to be installed from cran. Maybe we should change it to just install_load_packages? In that case, we would want to update the function call in inst/rmarkdown/templates/visc_empty/skeleton/skeleton.Rmd and inst/rmarkdown/templates/visc_report/skeleton/skeleton.Rmd.

slager commented 3 weeks ago

If install_load_cran_packages() is something that people have frequently used in their scripts, etc., we should be careful about renaming the function / changing the API. I suppose we could deprecate the old function name, having it still work but warn/remind them to use the new function name instead, and maybe eventually remove it.

In the motivating use case of using the Rstudio/Posit public package manager, they would still be CRAN packages, just compiled and built from source on Posit's computers instead of on the user's computer.

Depending on the above, I probably lean towards not changing the function name unless we start seeing use cases where people are using it to install non-CRAN packages, but I don't feel strongly either way. Happy to add renaming to the PR, with or without soft deprecating the old one, if you prefer that!

kelliemac commented 2 weeks ago

ok! in that case maybe we can leave the function name as is, and just add a comment to the code noting what the typical repos used for installation are, as you've described here?

slager commented 2 weeks ago

Added a note about CRAN/repos to the function comments