From 530e922eb3fecd74c3bd2232c97382ecbd6cf10c Mon Sep 17 00:00:00 2001 From: Xianghui Dong Date: Thu, 23 Feb 2017 15:03:10 -0500 Subject: [PATCH 1/5] added support for pacman::p_load package loaders --- R/dependencies.R | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/R/dependencies.R b/R/dependencies.R index 4710e693..4e52e187 100644 --- a/R/dependencies.R +++ b/R/dependencies.R @@ -359,11 +359,23 @@ identifyPackagesUsed <- function(call, env) { return() fn <- call[[1]] - if (!anyOf(fn, is.character, is.symbol)) - return() fnString <- as.character(fn) + # brute check for pacman package loader. the match.call method doesn't work with pacman p_load . Instead we just scan as.character(call). Since all other parameters in p_load are logical, we can exclude them by simple pattern matching and get the package list. + # put code before the :: check because pacman::p_load is often used. + pacmanLoaders <- c("p_load") + if (pacmanLoaders %in% fnString) { + s <- as.character(call) + sParas <- s[2:length(s)] + filterOut <- c("TRUE", "FALSE", "T", "F") + pkgs <- sParas[!sParas %in% filterOut] + lapply(pkgs, function(x) env[[x]] <- TRUE) + } + + if (!anyOf(fn, is.character, is.symbol)) + return() + # Check for '::', ':::' if (fnString %in% c("::", ":::")) { if (anyOf(call[[2]], is.character, is.symbol)) { From 47c48e7ed6ac81ef4f95cd5e7e29f29bd994d1d7 Mon Sep 17 00:00:00 2001 From: Xianghui Dong Date: Fri, 24 Feb 2017 09:28:44 -0500 Subject: [PATCH 2/5] Rmd and test file --- packrat_PR.Rmd | 71 +++++++++++++++++++++++++++++++++++++++++++++ tests/test-pacman.R | 18 ++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 packrat_PR.Rmd create mode 100644 tests/test-pacman.R diff --git a/packrat_PR.Rmd b/packrat_PR.Rmd new file mode 100644 index 00000000..c9d05bfd --- /dev/null +++ b/packrat_PR.Rmd @@ -0,0 +1,71 @@ +--- +title: "Untitled" +output: html_document +--- + +```{r setup, include=FALSE} +knitr::opts_chunk$set(echo = TRUE) +``` + +The character.only usage will not be recognized in current code. Though probably this usage for library is seldom used. It's possible to use this usage with pacman because it can accept a vector of package names, but again it should be less used because user can input package names without quote directly, unless they want the package list generated by program. + + x <- "ggplot2" + library(x, character.only = TRUE) + +match.call doesn't work with p_load, loader will get the function(pkg, name) +use brute force of pattern matching in string instead of language parsing. + +p_load_gh need similar process with devtools line, may need to edit in other places. just use devtools instead. + +note pacman package itself will be picked up by original code no matter what: +if used pcaman::p_load, will pick up because of :: +if used library(pacman), will pick up because of library. + +supported syntax + + pacman::p_load(stringr, units, install = FALSE) + pacman::p_load(shiny, shinydashboard) + +unsupported syntax +- arguments that saved in variable. + + op <- TRUE + pacman::p_load(shinyjs, install = op) + pkgs <- c("dplyr", "tibble") + pacman::p_load(pkgs, character.only = TRUE) + +- similar usage for library cannot be detected with original packrat either + + pkg <- "ggmap" + library(pkg, character.only = TRUE) + +```{r} +library(packrat) +file <- "/Users/xhdong/Projects/ctmm-explorations/shiny/dashboard1/app.R" +exprs <- suppressWarnings(parse(file, n = -1L)) +e <- exprs[[3]] +env <- new.env(parent = emptyenv()) +call <- e +packrat:::identifyPackagesUsed(e, env) + +p <- "pacman::p_load(ggplot2,ctmm)" +p <- "p_load(ggplot2, install = FALSE)" +exprs <- suppressWarnings(parse(text = p, n = -1L)) +e <- exprs2[[1]] +call <- e +env <- new.env(parent = emptyenv()) +packrat:::identifyPackagesUsed(e, env) + +s <- as.character(call) +sParas <- s[2:length(s)] +filterOut <- c("TRUE", "FALSE", "T", "F") +sParas[!sParas %in% filterOut] +``` + +test +```{r} +packrat:::fileDependencies(file) +packrat:::fileDependencies("tests/test-pacman.R") + +``` + diff --git a/tests/test-pacman.R b/tests/test-pacman.R new file mode 100644 index 00000000..bc1d720d --- /dev/null +++ b/tests/test-pacman.R @@ -0,0 +1,18 @@ +# test pr for pacman support +if (!require("pacman")) install.packages("pacman") +# this doesn't work, use devtools before running instead. +# pacman::p_load_gh("ctmm-initiative/ctmm") +pacman::p_load(shiny, shinydashboard, DT, ctmm, ggplot2, scales, gridExtra, data.table, lubridate, markdown) + +pacman::p_load(stringr, units, install = FALSE) + +op <- TRUE +pacman::p_load(shinyjs, install = op) + +pkgs <- c("dplyr", "tibble") +pacman::p_load(pkgs, character.only = TRUE) + +library(bit64) + +pkg <- "ggmap" +library(pkg, character.only = TRUE) From 9dfd82638d23c7dfe81bed9fdfa1cfb332caeaea Mon Sep 17 00:00:00 2001 From: Xianghui Dong Date: Fri, 24 Feb 2017 10:13:52 -0500 Subject: [PATCH 3/5] clean up documentation --- R/dependencies.R | 5 ++++- packrat_PR.Rmd | 26 ++++++++++++++++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/R/dependencies.R b/R/dependencies.R index 4e52e187..d9e5d4e8 100644 --- a/R/dependencies.R +++ b/R/dependencies.R @@ -362,7 +362,10 @@ identifyPackagesUsed <- function(call, env) { fnString <- as.character(fn) - # brute check for pacman package loader. the match.call method doesn't work with pacman p_load . Instead we just scan as.character(call). Since all other parameters in p_load are logical, we can exclude them by simple pattern matching and get the package list. + # adding support for pacman package loader + # https://github.com/rstudio/packrat/pull/360 + # The method of line `matched <- match.call(loader, call)` doesn't work with p_load, because `p_load` doesn't have the package list as the named argument. + # Because all other arguments in `p_load` will be logical, supposedly be one of `True, False, T, F`, so I just scan `as.character(call)` to get the package name list. # put code before the :: check because pacman::p_load is often used. pacmanLoaders <- c("p_load") if (pacmanLoaders %in% fnString) { diff --git a/packrat_PR.Rmd b/packrat_PR.Rmd index c9d05bfd..b6bb7ed1 100644 --- a/packrat_PR.Rmd +++ b/packrat_PR.Rmd @@ -1,21 +1,18 @@ --- -title: "Untitled" +title: "pull request for adding pacman support" output: html_document --- ```{r setup, include=FALSE} knitr::opts_chunk$set(echo = TRUE) ``` +# implementation -The character.only usage will not be recognized in current code. Though probably this usage for library is seldom used. It's possible to use this usage with pacman because it can accept a vector of package names, but again it should be less used because user can input package names without quote directly, unless they want the package list generated by program. +The method of line `matched <- match.call(loader, call)` doesn't work with p_load, because `p_load` doesn't have the package list as the named argument. - x <- "ggplot2" - library(x, character.only = TRUE) +Because all other arguments in `p_load` will be logical, supposedly be one of `True, False, T, F`, so I just scan `as.character(call)` to get the package name list. -match.call doesn't work with p_load, loader will get the function(pkg, name) -use brute force of pattern matching in string instead of language parsing. - -p_load_gh need similar process with devtools line, may need to edit in other places. just use devtools instead. +p_load_gh is not implemented because I had some problems with using it to install some github repo. I need to use devtools anyway. note pacman package itself will be picked up by original code no matter what: if used pcaman::p_load, will pick up because of :: @@ -39,7 +36,7 @@ unsupported syntax pkg <- "ggmap" library(pkg, character.only = TRUE) -```{r} +```{r exploration code used in development} library(packrat) file <- "/Users/xhdong/Projects/ctmm-explorations/shiny/dashboard1/app.R" exprs <- suppressWarnings(parse(file, n = -1L)) @@ -63,9 +60,18 @@ sParas[!sParas %in% filterOut] ``` test +The `test-pacman.R` doesn't follow the testthat rule, which is just a source file with different pacman syntax to test the code. Proper test code can be added later if needed. ```{r} packrat:::fileDependencies(file) packrat:::fileDependencies("tests/test-pacman.R") - ``` +## shiny deployment +if just build and reload package, shiny deployment will have error: + + Error: Unable to retrieve package records for the following packages: + 'packrat' + +It's because packages either from CRAN or github, if github, must installed by devtools. And packrat is in implicit dependency. + +I pushed my changes to my github repo, install packrat from there. Now everything worked, including pacman loader, and github installed packages. the install.package line doesn't hurt either. From 557211be1df031f36d1873f509d31b99d07640b6 Mon Sep 17 00:00:00 2001 From: dracodoc Date: Sun, 26 Feb 2017 14:47:29 -0500 Subject: [PATCH 4/5] move PR related file and test code to inst folder, avoid build errors caused by test code inside testthat folder --- packrat_PR.Rmd => inst/PR-360/packrat_PR.Rmd | 8 +++--- inst/PR-360/test.Rmd | 27 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) rename packrat_PR.Rmd => inst/PR-360/packrat_PR.Rmd (90%) create mode 100644 inst/PR-360/test.Rmd diff --git a/packrat_PR.Rmd b/inst/PR-360/packrat_PR.Rmd similarity index 90% rename from packrat_PR.Rmd rename to inst/PR-360/packrat_PR.Rmd index b6bb7ed1..199b010b 100644 --- a/packrat_PR.Rmd +++ b/inst/PR-360/packrat_PR.Rmd @@ -60,10 +60,12 @@ sParas[!sParas %in% filterOut] ``` test -The `test-pacman.R` doesn't follow the testthat rule, which is just a source file with different pacman syntax to test the code. Proper test code can be added later if needed. + +sample code is put inside a Rmarkdown because they are not suitable for regular testtaht folder. + ```{r} -packrat:::fileDependencies(file) -packrat:::fileDependencies("tests/test-pacman.R") +# packrat:::fileDependencies(file) +packrat:::fileDependencies("inst/PR-360/test.Rmd") ``` ## shiny deployment diff --git a/inst/PR-360/test.Rmd b/inst/PR-360/test.Rmd new file mode 100644 index 00000000..02ef0636 --- /dev/null +++ b/inst/PR-360/test.Rmd @@ -0,0 +1,27 @@ +Run test with +`packrat:::fileDependencies("inst/PR-360/test.Rmd")` + +Note `rmarkdown` was added to list because this file is RMarkdown. + +```{r} +# test pr for pacman support +if (!require("pacman")) install.packages("pacman") +# this doesn't work, use devtools before running instead. +# pacman::p_load_gh("ctmm-initiative/ctmm") +pacman::p_load(shiny, shinydashboard, DT, ctmm, ggplot2, scales, gridExtra, data.table, lubridate, markdown) + +pacman::p_load(stringr, units, install = FALSE) + +op <- TRUE +pacman::p_load(shinyjs, install = op) + +pkgs <- c("dplyr", "tibble") +pacman::p_load(pkgs, character.only = TRUE) + +library(bit64) + +pkg <- "ggmap" +library(pkg, character.only = TRUE) + +``` + From 89a77b023080eba4571f6d621da4694a8b212351 Mon Sep 17 00:00:00 2001 From: Xianghui Dong Date: Mon, 27 Feb 2017 10:03:29 -0500 Subject: [PATCH 5/5] edit in rmd --- packrat_PR.Rmd | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packrat_PR.Rmd b/packrat_PR.Rmd index b6bb7ed1..4ed7063f 100644 --- a/packrat_PR.Rmd +++ b/packrat_PR.Rmd @@ -12,7 +12,7 @@ The method of line `matched <- match.call(loader, call)` doesn't work with p_loa Because all other arguments in `p_load` will be logical, supposedly be one of `True, False, T, F`, so I just scan `as.character(call)` to get the package name list. -p_load_gh is not implemented because I had some problems with using it to install some github repo. I need to use devtools anyway. +`p_load_gh` is not implemented because the github repo string could be quite complicated. Instead user just need to load that package in `p_load` again, which will make sure the packages be picked up by `packrat`, and there is no negative impact of this. note pacman package itself will be picked up by original code no matter what: if used pcaman::p_load, will pick up because of ::