search

leekgroup / recount

R package for the recount2 project. Documentation website: http://leekgroup.github.io/recount/
https://jhubiostatistics.shinyapps.io/recount/
40 stars 9 forks source link

Remove package installation code #14

Closed LiNk-NY closed 5 years ago

LiNk-NY commented 5 years ago

Hi Leonardo, @lcolladotor Please remove package installation code from recount. Assume that the package is installed. If said package is in the Suggests fields and it is required for a function, check using requireNamespace, then stop if the package is not available, and ask the user to install it first. Do not install the package for the user.

https://github.com/leekgroup/recount/blob/8da982b309e2d19638166f263057d9f85bb64e3f/R/utils.R#L14-L32 Best, Marcel

lcolladotor commented 5 years ago

Hi Marcel @LiNk-NY,

That function comes from regionReport which I added based on Karthik's feedback on his peer review report for regionReport at https://f1000research.com/articles/4-105/v1#referee-response-8558.

I'll change this a bit so it'll still give the user the list of packages they need to install before stopping, in case there are multiple packages so they don't have to go through multiple rounds of errors.

Best, Leo

LiNk-NY commented 5 years ago

Hi Marcel @LiNk-NY,

That function comes from regionReport which I added based on Karthik's feedback on his peer review report for regionReport at https://f1000research.com/articles/4-105/v1#referee-response-8558.

I'll change this a bit so it'll still give the user the list of packages they need to install before stopping, in case there are multiple packages so they don't have to go through multiple rounds of errors.

Best, Leo

Yes, it's good to move packages to Suggests when they are not essential to the codebase and to use requireNamespace for checking. But you shouldn't install packages for users. I would recommend to have users set the dependencies = TRUE argument in BiocManager::install to get all Suggests dependencies, if need be.

lcolladotor commented 5 years ago

Ahh, thanks Marcel, I didn't know about dependencies = TRUE. In any case, I think that the code should work well now and not mess with users' installed packages.

Best, Leo