hpages
commented
11 months ago Thanks @js2264 for the changes.
The easiest approach I can think of, in order not to have to download this archive from GitHub, would be to include this template as extdata in BiocBook package itself
Definitely some advantages to this approach:
- Would prevent repeated downloads of the template. For example, once the BiocBook package gets added to the Bioconductor daily builds, the template will get downloaded twice a day (one during the release builds, and one during the devel builds) when BBS runs
R CMD checkon the package. - Another advantage of this approach is that it creates a hard tie between the template and the software. This eliminates the risk of running into a situation where changes in the software also require changes in the template that are not backward compatible with earlier versions of the software.
- It would allow the user to work offline, granted they call
init()withskip_availability=TRUEand.local=TRUE. - Finally this would also make the
init()interface slightly simpler by getting rid of theversionargument. Thetemplateargument could stay, but I'm not sure what it would become. Maybe the path or URL to an alternative template? Default value could beNULL, which would mean "use the template included in the package". Another option is to simply get rid of thetemplateargument.
I can't think of any drawbacks to this approach. The template is small (v1.0.4.tar.gz is 152K) so size is not a concern.
Note that one could argue that a package template is not really data so feel free to put this anywhere under inst/.
The recommendation for a software package is to set the URL field to the canonical URL of its landing page (a.k.a. "Package Short URL"), which will be https://bioconductor.org/packages/BiocBook in the case of BiocBook (the page does not exist yet but it will get automatically created after the package gets added to our daily builds). The BugReports field already points people to the issue tracker on GitHub. Bioconductor prefers to make the package landing page the primary URL, and not advertize the URL of the Github repo too much because this encourages people to install the package from there (which is bad) or to use the issue tracker to ask questions about how to use the software (which is also bad).
Also I would suggest to set the BugReports field to the issue part of the GitHub repo: https://github.com/js2264/BiocBook/issues
Found in BiocBook/README.md:
To install
BiocBookdevelopment version, you can use:install.packages("devtools") devtools::install_github("js2264/BiocBook")
You should still not do this. If you want to install the development version, you first need to make sure that you are using the development version of Bioconductor (depending on the time of the year, this might require installing R devel), then doing BiocManager::install("BiocBook") will bring the development version of BiocBook. As mentioned earlier, installing directly from GitHub has the potential to mess up your installation by introducing version mismatches. In other words, BiocManager::install() is the only safe way to install Bioconductor packages.
Similar problem found in BiocBookDemo/inst/pages/Chapter-2.qmd:
if (!require(BiocBook)) BiocManager::install('js2264/BiocBook')Not sure why I get the following message when I try to use an existing name:
> init('OSCA', .local = TRUE)
...
! `available` server is currently unavailable. Skipping validation...
...Also shouldn't this be a hard error? I.e. the function should stop and not create the OSCA folder.
I find the name of the commit argument misleading. IIUC this is about pushing commits, not committing changes. Also the default for commit is NA but the man page says it's FALSE. Furthermore, based on the code, it seems that commit=NA and commit=FALSE don't have the same meaning but the man page does not document the meaning of the former.
Given that the BiocBook man page (?BiocBook) documents more than one function, the \value section should either document them all, or at least clarify which one it refers to. Right now it's up to the user to figure this out:
Value:
A ‘BiocBook’ object (invisible).No need to re-implement tempfile(). Instead of:
tmpdir <- paste0('.', paste0(
sample(c(seq(0, 9), LETTERS), 8, replace = TRUE), collapse = ""
))
dir.create(tmpdir)just do something like:
tmpdir <- tempfile() # or tempfile("BiocBook.template."), but it does not matter _at all_
dir.create(tmpdir)Note that an important feature of tempfile() is that it returns the path to a file that will be created inside tempdir(). This means that the file is guaranteed to get removed at the end of the R session. Right now your code explicitely needs to take care of that (with unlink(tmpdir, recursive = TRUE)). However, if for whatever reason the call to init() dies prematurely, the tmp dir that your code created in the working directory will remain there, hence contaminating the user's folder with a stale hidden subfolder. See ?tempfile for more info.
This seems like a lot of code only to copy the content of one folder to an empty folder:
## Move files from temp folder to `new_package` folder
d <- list.dirs(tmpdir, full.names = TRUE, recursive = TRUE)
d <- d[dirname(d) != '.']
pattern <- file.path(tmpdir, basename(d)[dirname(dirname(d)) == '.'])
d <- d[dirname(dirname(d)) != '.']
f <- list.files(
tmpdir,
all.files = TRUE, full.names = TRUE, recursive = TRUE
)
f <- f[!grepl("archive.zip$", f)]
for (.d in d) dir.create(gsub(pattern, repo, .d))
for (.f in f) file.copy(from = .f, to = gsub(pattern, repo, .f))Is this because the various files that need to be skipped (the *archive.zip files) can be located at any depth in the tempdir tree? Otherwise copying the content can be done with:
content <- list.files(tmpdir, all.files=TRUE, full.names=TRUE, no..=TRUE)
file.copy(content, repo, recursive=TRUE)If the file to skip is located at the top of the tmpdir tree, then add:
content <- content[!grepl("archive.zip$", content)]before calling file.copy().
Not sure what the purpose of the invisible(BiocBook(repo)) line is here:
if (!is.na(commit)) {
if (commit) {
gert::git_push(repo = repo)
cli::cli_alert_success(cli::col_grey("Commits pushed to origin {cli::col_cyan(gert::git_remote_list(repo = repo)$url[1])}"))
}
else {
invisible(BiocBook(repo))
}
}
...The if statement is not the last expression in the body of the init() function so the invisible(BiocBook(repo)) line won't return anything. It's just a no-op.
The body of the init() function is very long. Readbility, troubleshooting, and long-term maintainability, could be improved significantly by breaking it down into smaller internal helper functions, one for each major step accomplished by the big function.
Thanks again, H.
bioc-issue-bot
js2264
lshep
Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor
Confirm the following by editing each check box to '[x]'
[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.
[x] I am familiar with the Bioconductor code of conduct and agree to abide by it.
I am familiar with the essential aspects of Bioconductor software management, including:
For questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.