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

[FR] Simplify version checking mechanism #9

Closed
LiNk-NY opened this issue Sep 1, 2020 · 7 comments
Closed

[FR] Simplify version checking mechanism #9

LiNk-NY opened this issue Sep 1, 2020 · 7 comments

Comments

@LiNk-NY
Copy link
Contributor

LiNk-NY commented Sep 1, 2020

Hi Leo, @lcolladotor

I think we can make the bioc_check.yaml file more simple since we are invoking the function
from R to generate the file. We can use the R instance to dictate the version of the docker images.

For example, if the developer runs biocthis::use_bioc_github_action() with a Bioc-devel set-up, the generated
yaml file should use bioconductor/bioconductor_docker:devel.
This will remove the need to create a step to verify the appropriate R version.

https://github.com/lcolladotor/biocthis/blob/master/actions/check-bioc.yml

Also, I think this file should be under some folder like inst/templates/check-bioc.yml.
The code can then use glue or whiskers or whatever package people use nowadays to replace
the Bioconductor version in the yaml file after doing system.file(..., package = "biocthis")

(Related to #8)

Best,
Marcel

@lcolladotor
Copy link
Owner

lcolladotor commented Oct 1, 2020

Hm. I guess that I could make the template use conditional code. Like a biocthis::use_bioc_github_action(dynamic = TRUE) which would use the current version, and if dynamic = FALSE then it would use the proposed workflow.

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Oct 1, 2020

Hi Leo, @lcolladotor
I've taken a stab at this in PR #11.
It would use the user's Bioconductor version (whether release or devel) to fill in the GHA template.

@lcolladotor
Copy link
Owner

I'm just going through that PR, it's really fancy!! Thanks Marcel!!

@lcolladotor
Copy link
Owner

lcolladotor commented Oct 1, 2020

Thank you Marcel for the great PR! After a bunch of tests at https://github.com/lcolladotor/testmatrix and a few commits on biocthis, I think that we can close this issue.

Note that I had to revert some of the code from your PR #11: the sysreqs part is one of them. This commit shows most of the changes 66a6232. Something else I used was remotes::install_cran("rcmdcheck") which doesn't update if it detects that the latest version is there already.

I also added arguments to biocthis::use_bioc_github_action() to control setting the main environment variables (pkgdown, covr, RUnit, testhat) on the GHA workflow. I added to the R package some of the previous GHA code for finding the R version that matched a given BioC version so .GHARversion() looks quite different.

I'll close this issue, but we might want to revise some details here and there. Anyway, this version works now ^^

I added you as a ctb ^^ Hopefully that's ok =)

@LiNk-NY
Copy link
Contributor Author

LiNk-NY commented Oct 1, 2020

Thanks for looking it over Leo! @lcolladotor

I agree with all the changes required to make it work on non-Linux platforms. I didn't have a chance to test it 😅

I don't think the .GHARversion has to be that complicated given the use cases as detailed in previous responses. Perhaps they weren't clear so I will provide an actual example.

Suppose I am using Bioconductor devel and I would like to add / update the GitHub Action for my repository.
I go into my session with the corresponding setup which has this Bioconductor and R version:

> BiocManager::version()
[1] '3.12'
# AND
> getRversion()
[1] '4.0.2'

Note. My package checkout should always correspond to the branch that I am in:

~/bioc/biocthis (master) $ git branch
* master

image

For the Bioconductor release scenario, a typical setup would be like this and
using a hypothetical release version of biocthis:

> BiocManager::version()
[1] '3.11'
# AND
> getRversion()
[1] '4.0.2'
~/bioc/biocthis (RELEASE_3_11) $ git branch
* RELEASE_3_11
  master

image

So it would be configured properly if the developer is using the appropriate setup 😉.
When a Bioc release is out, BiocManager should always be the source of the correct R version and Bioc version, when
BiocManager::valid() is TRUE.

Great idea to have optional modules such as:

pkgdown = TRUE,
    testthat = TRUE,
    covr = testthat,
    RUnit = FALSE

IMO, I'd make pkgdown and testthat FALSE by default. I think the aim is to get the testing done and have the user add any additional features at their leisure.
I'd also restrict pkgdown to only the release branch since devel changes often and documentation isn't really published for devel branches.
Thanks!

@lcolladotor
Copy link
Owner

So it would be configured properly if the developer is using the appropriate setup 😉.

Hehe true true. Though, I've also written packages say on Bioc-release and used Travis (and now GHA) on Bioc-devel to make sure it's all good. So ehem, this more complicated .GHARversion() helps me... sometimes be lazy ^^'. I agree with you that for the scenario where a developer is working in the correct setup, that there's no need to have the complicated .GHARversion().

About pkgdown and testthat, I use them in all my packages (ok, some have very low coverage values, ooops!). I see your points though, and maybe one way is to use getOption() so I can configure my .Rprofile to match my preferences. I have found pkgdown to be really helpful to check the rendered documentation of my package. It makes it easier to read and to identify typos and things like that. Creating it automatically really helps speed things up also. I also really like how pkgdown provides links, which makes the documentation much easier to navigate, hence why I have pkgdown websites for all my BioC packages.

Setting up a pkgdown release and devel would need more work, unlike what we have at BioC. Because I'm making changes that users will get to see in the next release version, I want to proof-read them and all that, that's why my docs use the devel branch and not the release one. Though well, after using biocthis::use_bioc_github_action(), a user can re-configure the GHA workflow. Hm... I guess that I could add a pkgdown_branch argument to the template function though.

@lcolladotor
Copy link
Owner

I added the getOption() code and a pkgdown_covr_branch argument which by default is master.

lcolladotor added a commit that referenced this issue Oct 1, 2020
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

No branches or pull requests

2 participants