search

Bioconductor / BiocCheck

http://bioconductor.org/packages/BiocCheck
8 stars 26 forks source link

Clarification on the regex used for detecting installation (of R packages?) on the vignette #192

Closed lcolladotor closed 10 months ago

lcolladotor commented 1 year ago

Hi,

The regex used to detect installations on vignettes at https://github.com/Bioconductor/BiocCheck/blob/ef289ebbaaec14ade634a73b29a2cdd2aa8f0846/R/checks.R#L881-L900 recognized a function in megadepth called install_megadepth() at https://github.com/LieberInstitute/megadepth/blob/2383d3a645d9cb554a546ffad4e21a2547329e8e/vignettes/megadepth.Rmd#L129 which doesn't install R software but does download the megadepth binary from https://github.com/ChristopherWilks/megadepth. See https://github.com/LieberInstitute/megadepth/actions/runs/4904717884/jobs/8758015209#step:24:69.

I'm not sure whether you would consider this a BiocCheck() error or not. I can disable this error on GitHub Actions and well, megadepth is already part of Bioconductor.

Anyway, I imagine that running megadepth::install_megadepth() on the vignette is still ok. If the issue is the name of the function, we could call it download_megadepth() instead.

Best, Leo

LiNk-NY commented 1 year ago

Hi Leo, @lcolladotor

IMO, the function is doing too much for the user and too much in the vignette.

I would follow the AnVIL package's example with system dependencies e.g., gcloud and write the R code assuming (and checking) that the system dependency is satisfied. If it isn't, you can still provide helpful instructions to the user to install megadepth.

OTOH, if the package's only motive is to install an external dependency, it would be more acceptable to have installation functions as in the case of basilisk, for example. The change in function name would also be justified.

Best, Marcel

LiNk-NY commented 10 months ago

I hope that answers the question. Feel free to reopen if it doesn't. Thanks!