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

Feature: Link to bioconductor vignettes #145

Merged
merged 17 commits into from
Jun 23, 2022

Conversation

zeehio
Copy link
Contributor

@zeehio zeehio commented Jun 21, 2022

This merge request provides support to parse vignette(...) calls that call vignettes from Bioconductor packages.

With the latest CRAN release version, a link is formed that returns a 404 error:

downlit::autolink('vignette("Introduction_To_BiocParallel", package = "BiocParallel")')
# [1] "<a href='https://cran.rstudio.com/web/packages/BiocParallel/vignettes/Introduction_To_BiocParallel.pdf'>vignette(\"Introduction_To_BiocParallel\", package = \"BiocParallel\")</a>"

With this pull request, we get the correct link:

downlit::autolink('vignette("Introduction_To_BiocParallel", package = "BiocParallel")')
# [1] "<a href='https://bioconductor.org/packages/release/bioc/vignettes/BiocParallel/inst/doc/Introduction_To_BiocParallel.pdf'>vignette(\"Introduction_To_BiocParallel\", package = \"BiocParallel\")</a>"

I went through a rabbit hole the last afternoon starting from pkgdown until I got here, I hope I can help get this fixed.

@hadley
Copy link
Member

hadley commented Jun 22, 2022

Is there possibly an easier way to detect if a package is from BioConductor by inspecting packageDescription()?

@zeehio
Copy link
Contributor Author

zeehio commented Jun 23, 2022

Oh yes, I did not realize that to get the path to the vignette we already required the package to be installed.

This simplifies a lot the code. Thanks!

I suggest to squash the commits when merging into main.

R/link.R Outdated
# Returns NA if package is not installed.
# Returns TRUE if `package` is from Bioconductor, FALSE otherwise
is_bioc_pkg <- function(package) {
desc <- suppressWarnings({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the warning that you are suppressing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the package is missing, packageDescription returns NA, but it also triggers a warning:

> packageDescription("potato")
[1] NA
Warning message:
In packageDescription("potato") : no package 'potato' was found

Would you prefer that I check if the package is installed via rownames(installed.packages()) instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since downlit already depends on rlang, you can use rlang::is_installed(). I think it's better to switch based on that, rather than suppressing the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for all your time and the feedback!

R/link.R Outdated
# Returns TRUE if `package` is from Bioconductor, FALSE otherwise
is_bioc_pkg <- function(package) {
if (!rlang::is_installed(package)) {
return(NA)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be simpler if you made this FALSE; then you wouldn't need the isTRUE() above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@hadley hadley merged commit f159cdd into r-lib:main Jun 23, 2022
@hadley
Copy link
Member

hadley commented Jun 23, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants