Bioconductor / BiocStyle

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

Behavior of BiocStyle::Biocpkg() #112

Open federicomarini opened 3 weeks ago

federicomarini commented 3 weeks ago

Referring to this issue: https://github.com/iSEE/iSEEde/issues/43

My thought was that this is indeed resulting from using BiocStyle::Biocpkg("iSEEde") in the Rmd README of the package.

Shouldn't the ideal behavior of this function be to use a non-versioned link? Instead of returning https://bioconductor.org/packages/3.16/iSEEde, wouldn't https://bioconductor.org/packages/iSEEde be the better option? Or alternatively, have the version info included as result of a parameter, e.g. include_version?

Why - because https://bioconductor.org/packages/3.16/iSEEde ends up "becoming broken" (but the "current one", https://bioconductor.org/packages/3.19/iSEEde would still work (probably as long as the current release is matched to that value?))

If I am missing something relevant about this being the default, let me know. Otherwise: happy to come in with a PR 😉

kevinrue commented 3 weeks ago

Maybe instead of include_version= I'd suggest an argument short_link=TRUE|FALSE. Not sure about the best default value.

aoles commented 3 weeks ago

Thanks @federicomarini for pointing this out! ❤️

I believe in the context of vignettes and reproducibility the inclusion of package version in the links makes sense. As for general links to the package landing page such as ones in e.g. README documents etc. it might indeed be reasonable to have an option of including an unversioned link.

My suggestion would be to introduce an argument versioned = TRUE to BiocStyle::Biocpkg. In your example you could then write

BiocStyle::Biocpkg("iSEEde", versioned = FALSE)

Feel free to submit a PR, I'm more than happy to review it 🚀

federicomarini commented 3 weeks ago

On it!

federicomarini commented 3 weeks ago

I just created a small PR to implement this functionality - feel free to edit if you fancy another coding style (I tried to go the compact way but could not shorten it up further)