Bioconductor / BiocStyle

Issues and pull requests for BiocStyle should go here.
12 stars 19 forks source link

Allow linking to workflow packages and vignettes/sections thereof #51

Closed LTLA closed 6 years ago

LTLA commented 6 years ago

Introduces a Biocworkpkg function that behaves like Biocpkg by default, but can also support linking to specific vignettes and specific sections of a vignette. In response to #46.

I also took the liberty of modifying Biocpkg and friends so that they dynamically switch between release and devel based on the current version of BiocStyle.

LTLA commented 6 years ago

Nudging for some feedback - good? Bad? Crazy?

aoles commented 6 years ago

Thank you @LTLA for your contribution! I like the idea of a macro for linking to workflow packages, especially the ability to link to vignettes at section level. This approach could be even potentially generalized to the rest of the Biocpkg family functions as well. The only possible problem I see is link rot if the maintainer of the linked package decides to change the vignette name or structure.

I'm not sure, though, whether the decision to which version of BioC to resolve to should be based on the version of BiocStyle. A more robust approach would probably be to use BiocInstaller::isDevel() for this. Note that in the current implementation Biocpkg uses as link the short pkg url. This has the advantage that even though by default it resolves to release packages from devel not yet in release will still be picked up.

LTLA commented 6 years ago

Thanks @aoles. I hadn't thought much about link rot, mostly because I was using the functions to link between vignettes of the same workflow; in such cases, the only person to blame would be myself, if I change the vignette file name and forget to change the links. I can imagine that this would be more difficult if I was linking to other people's vignettes - however, I think that R CMD check will report invalid links in compiled vignettes, so at least the maintainer will have a chance to fix things.

As for the version number; yes, I agree, I only used the package version to avoid putting in another dependency until I got your opinion on things. Also I wasn't sure about the future of BiocInstaller now that BiocManager is on CRAN. The use of versioning was motivated by the fact that my devel workflows are often heavily modified compared to the release version, so I'd like the links in my devel documents to point to other devel vignettes. New packages should be handled properly under this scheme, as the only documents that should ever refer to new packages should themselves be in devel.

Finally, I realized that the section argument (or whatever I called it, I can't quite remember) is probably overkill; it should be possible to do it outside the function, like:

blah blah blah [here](`r Biocworkpkg("simpleSingleCell", "work-1-intro.Rmd")`#section-link)

... which would take less typing anyway.

LTLA commented 6 years ago

As it turns out, BiocVersion is probably the best way forward to get the version information. @mtmorgan, could you check whether the usage is sensible here?

LiNk-NY commented 6 years ago

Hi Aaron, @LTLA If you have to go the BiocInstaller route, I suggest you use the BiocManager helper function (.version_map) here: https://github.com/Bioconductor/BiocManager/blob/master/R/version.R#L35 in combination with BiocVersion to test for devel. PS. Based on the name, the function seems like it should TRUE or FALSE Regards, Marcel

LTLA commented 6 years ago

Thanks Marcel. I realized that the Bioconductor website links work equally well with the explicit numbered version of Bioconductor (e.g., "3.8") in place of "release" or "devel", so there's no need to resolve the former to the latter anymore. I guess I should also rename the is_devel function as well.

LTLA commented 6 years ago

I don't know why the different Bioc*pkg() types were written initially, but currently they've been quite useful at allowing me to point to versioned URLs for the annotation/experiment packages. Previously, the different types simply called Biocpkg(), which would point towards the general URL for a package, e.g., https://bioconductor.org/packages/scran. If I wanted to point to a specific version, I wasn't able to get that with a single Biocpkg() call (at least, not at the time, or not to my knowledge).

Obviously, if the website is getting restructured, this could be fixed at the source.

LTLA commented 6 years ago

Any further thoughts on this?

mtmorgan commented 6 years ago

The web site will be revised, so from my perspective I'd say hold on... Technically it is on our sprint board for this sprint, but the sprint is crowded with the conference.

mtmorgan commented 6 years ago

Links like

https://bioconductor.org/packages/affy/vignettes/affy.pdf
https://bioconductor.org/packages/devel/affy/vignettes/affy.pdf
https://bioconductor.org/packages/release/affy/vignettes/affy.pdf
https://bioconductor.org/packages/3.8/affy/vignettes/affy.pdf

redirect to the full vignette path, regardless of whether the package is software / data annotation / data experiment / workflow, just as https://bioconductor.org/packages/affy etc direct to package landing pages.

I think the way forward here is really to update Biocpkg() to accept an optional 'vignette' argument which, when present, creates the link above. Probably this is in scope for the current pull request.

I think the other Bioc*pkg() functions should be discouraged (not publicly advertised in the vignette; flagged as discouraged in the man page) or better deprecated (but this requires a lot of clean-up in ohter packages...). Probably not in scope for the current pull request.

LTLA commented 6 years ago

Done.