add support for renv lockfiles (#55) #80
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## v0.2 #80 +/- ##
==========================================
+ Coverage 95.94% 96.65% +0.71%
==========================================
Files 6 6
Lines 690 718 +28
==========================================
+ Hits 662 694 +32
+ Misses 28 24 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
schochastics
changed the title
|
@chainsawriot can I get you opinion on the current implementation? Do you think this makes sense and is it in line with how you envision as_pkgrefs? |
|
@schochastics Thanks! Several suggestions in the review. |
|
@chainsawriot can you link your comments? I cant find suggestions anywhere |
|
@schochastics https://github.com/chainsawriot/rang/pull/80/files/0448b1fe9976267ec8ef782cc95b29878f7bbeb2 I just found that I need to submit my comments. Sorry for that. |
| .extract_pkgref_renv_lockfile <- function(path){ | ||
| lockfile <- .parse_renv_lockfile(path) | ||
| sources <- vapply(lockfile[["Packages"]],`[[`,character(1),"Source",USE.NAMES = FALSE) | ||
| pkgs <- c() |
There was a problem hiding this comment.
a bit better to name this pkgrefs
R/as_pkgrefs.R
Outdated
| if(isFALSE(file.exists(path))){ | ||
| return(FALSE) | ||
| } | ||
| if(grepl("renv.lock$",path)){ |
There was a problem hiding this comment.
if (isFALSE(grepl("renv.lock$", path))) {
return(FALSE)
}
TRUEThere was a problem hiding this comment.
A slightly more robust method is basename(path) == "renv.lock"
It prevent the matching of hello_1231212renv.lock
| @@ -91,3 +91,8 @@ test_that("as_pkgrefs_packageDescription", { | |||
| ## expect_equal(res, c("github::chainsawriot/grafzahl", "cran::rtoot", "local::/home/chainsawriot/dev/rang", "cran::testthat") | |||
| expect_equal(res, c("github::chainsawriot/grafzahl", "cran::rtoot", "cran::rang", "cran::testthat")) | |||
| }) | |||
|
|
|||
| test_that("as_pkgrefs renv_lockfile", { | |||
| res <- as_pkgrefs("../testdata/renv.lock") | |||
There was a problem hiding this comment.
This is another good test data
https://github.com/csbg/tfcf/blob/69567c968a6de01ca9389eb444811b735b430ef8/renv.lock#L11
There was a problem hiding this comment.
Add one more test
expect_false(.detect_renv_lockfile("./testdata/graph.RDS"))There was a problem hiding this comment.
expect_false(.detect_renv_lockfile(c("./testdata/graph.RDS", "../testdata/renv.lock")))Should this be false?
There was a problem hiding this comment.
at least from the logic I am using. Which is that a renv lockfile occurs as a the only imput. Should we accept vector input for renv lock files, i.e. mixing types?
There was a problem hiding this comment.
@schochastics I actually agree that this should be FALSE. A vector of lock files is too arcane a situation.
There was a problem hiding this comment.
jep thats what I thought
| @@ -212,7 +212,7 @@ | |||
| #' | |||
| #' This function recursively queries dependencies of R packages at a specific snapshot time. The dependency graph can then be used to recreate the computational environment. The data on dependencies are provided by R-hub. | |||
| #' | |||
| #' @param pkgs `pkgs` can be 1) a character vector of R packages to resolve, or 2) a data structure that [as_pkgrefs()] can convert to a character vector of package references. For 1) `pkgs` can be either in shorthands, e.g. "rtoot", "ropensci/readODS", or in package references, e.g. "cran::rtoot", "github::ropensci/readODS". Please refer to the [Package References documentation](https://r-lib.github.io/pkgdepends/reference/pkg_refs.html) of `pak` for details. Currently, this package supports only cran and github packages. For 2) [as_pkgrefs()] support the output of [sessionInfo()]. | |||
| #' @param pkgs `pkgs` can be 1) a character vector of R packages to resolve, 2) a path to a [`renv` lockfile](https://rstudio.github.io/renv/articles/lockfile.html), or 3) a data structure that [as_pkgrefs()] can convert to a character vector of package references. For 1) `pkgs` can be either in shorthands, e.g. "rtoot", "ropensci/readODS", or in package references, e.g. "cran::rtoot", "github::ropensci/readODS". Please refer to the [Package References documentation](https://r-lib.github.io/pkgdepends/reference/pkg_refs.html) of `pak` for details. Currently, this package supports only cran and github packages. For 2) [as_pkgrefs()] support the output of [sessionInfo()]. | |||
There was a problem hiding this comment.
It would be better to add an integration test to test_resolve.R
testthat("integration of renv to resolve", {
expect_error(X <- resolve("../testdata/renv.lock", snapshot_date = "2023-01-01"), NA)
### more things
})There was a problem hiding this comment.
@chainsawriot should I move all tests there or do a new set of tests?
There was a problem hiding this comment.
Just this test. In general, the last part of test_resolve.R is for all tests that require internet.
|
@chainsawriot I think I incorporated everything now |
still WIP