From 05fdd73a83309698391179e38a5591fe2c24c228 Mon Sep 17 00:00:00 2001 From: Duncan Murdoch Date: Thu, 31 Mar 2022 19:46:14 -0400 Subject: [PATCH 1/5] Don't assume all scripts are given as character variables. Fix the error message to only report missing files. --- NEWS.md | 3 +++ R/html_dependency.R | 12 +++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index 87eaa55d..c7b126f4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,6 +12,9 @@ * `copyDependencyToDir()` no longer creates empty directories for dependencies that do not have any files. (@gadenbuie, #276) +* Closed #320: `copyDependencyToDir()` now works with dependencies +with specified attributes. (@dmurdoch, #320) + # htmltools 0.5.2 ## Breaking Changes diff --git a/R/html_dependency.R b/R/html_dependency.R index 662a323c..c3c25b63 100644 --- a/R/html_dependency.R +++ b/R/html_dependency.R @@ -354,9 +354,11 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) { dir.create(target_dir) dependency$src$file <- normalizePath(target_dir, "/", TRUE) - files <- if (dependency$all_files) list.files(dir) else { - unlist(dependency[c('script', 'stylesheet', 'attachment')]) - } + files <- if (dependency$all_files) + list.files(dir) + else + vapply(unlist(dependency[c('script', 'stylesheet', 'attachment')], recursive = FALSE), + function(f) if (is.list(f)) f[['src']] else f, "") if (length(files) == 0) { # This dependency doesn't include any files @@ -366,11 +368,11 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) { } srcfiles <- file.path(dir, files) - if (any(!file.exists(srcfiles))) { + if (any(bad <- !file.exists(srcfiles))) { stop( sprintf( "Can't copy dependency files that don't exist: '%s'", - paste(srcfiles, collapse = "', '") + paste(srcfiles[bad], collapse = "', '") ) ) } From 0969fdfea41483ce38aa88e3f9b6ddda8b4a6dac Mon Sep 17 00:00:00 2001 From: Duncan Murdoch Date: Fri, 1 Apr 2022 04:45:56 -0400 Subject: [PATCH 2/5] Add test, and fix error that it found. --- R/html_dependency.R | 18 +++++++++++---- tests/testthat/test-deps.r | 47 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/R/html_dependency.R b/R/html_dependency.R index c3c25b63..e618f8a1 100644 --- a/R/html_dependency.R +++ b/R/html_dependency.R @@ -354,11 +354,19 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) { dir.create(target_dir) dependency$src$file <- normalizePath(target_dir, "/", TRUE) - files <- if (dependency$all_files) - list.files(dir) - else - vapply(unlist(dependency[c('script', 'stylesheet', 'attachment')], recursive = FALSE), - function(f) if (is.list(f)) f[['src']] else f, "") + if (dependency$all_files) + files <- list.files(dir) + else { + file_list <- c(dependency[['script']], + dependency[['stylesheet']], + dependency[['attachment']]) + if (anyUnnamed(file_list)) + files <- vapply(file_list, + function(f) if (is.list(f)) f[['src']] else f, + "") + else + files <- file_list[['src']] + } if (length(files) == 0) { # This dependency doesn't include any files diff --git a/tests/testthat/test-deps.r b/tests/testthat/test-deps.r index eef577ce..85ba395b 100644 --- a/tests/testthat/test-deps.r +++ b/tests/testthat/test-deps.r @@ -357,3 +357,50 @@ test_that("copyDependencyToDir() doesn't create an empty directory", { # to keep relativeTo() from throwing an error later in Rmd render process expect_match(copied_dep$src$file, normalizePath(tmp_rmd, "/", TRUE), fixed = TRUE) }) + +test_that("copyDependencyToDir() handles attributes", { + tmp_dep <- tempfile("dep") + dir.create(tmp_dep) + on.exit(unlink(tmp_dep)) + + tmp_txt <- basename(tempfile("text", fileext = ".txt")) + tmp_path <- file.path(tmp_dep, tmp_txt) + writeLines("Some text in the text/plain dep", tmp_path) + on.exit(unlink(tmp_path), add = TRUE) + + tmp_rmd <- tempfile("rmd_files") + dir.create(tmp_rmd) + on.exit(unlink(tmp_rmd), add = TRUE) + + dep_with_attributes <- htmltools::htmlDependency( + name = "textPlain", + version = "9.9.9", + src = tmp_dep, + script = list(src = tmp_txt, type = "text/plain"), + all_files = FALSE + ) + + dep_without <- htmltools::htmlDependency( + name = "textPlain", + version = "9.9.9", + src = tmp_dep, + script = tmp_txt, + all_files = FALSE + ) + + dep_with_both <- htmltools::htmlDependency( + name = "textPlain", + version = "9.9.9", + src = tmp_dep, + script = list(tmp_txt, + list(src = tmp_txt, type = "text/plain")), + all_files = FALSE + ) + # None of these should trigger errors as the first two did in + # issue #320 + + copyDependencyToDir(dep_with_attributes, tmp_rmd) + copyDependencyToDir(dep_with_both, tmp_rmd) + copyDependencyToDir(dep_without, tmp_rmd) + succeed() +}) From c2a6536adfa55f57e34144219b2ad14bfa3d66b8 Mon Sep 17 00:00:00 2001 From: dmurdoch Date: Fri, 1 Apr 2022 10:13:42 -0400 Subject: [PATCH 3/5] Update NEWS.md Co-authored-by: Carson Sievert --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index c7b126f4..81bbc7dd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -13,7 +13,7 @@ * `copyDependencyToDir()` no longer creates empty directories for dependencies that do not have any files. (@gadenbuie, #276) * Closed #320: `copyDependencyToDir()` now works with dependencies -with specified attributes. (@dmurdoch, #320) +with specified attributes. (@dmurdoch, #321) # htmltools 0.5.2 From 186762a60f5c14bc2c82ddb263db8a0451019ded Mon Sep 17 00:00:00 2001 From: Duncan Murdoch Date: Fri, 1 Apr 2022 11:39:53 -0400 Subject: [PATCH 4/5] Add pluck_src function to get filename, add extra tests. --- R/html_dependency.R | 15 +++------- R/utils.R | 9 ++++++ tests/testthat/test-deps.r | 58 ++++++++++++++++++++++++++++++++------ 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/R/html_dependency.R b/R/html_dependency.R index e618f8a1..99445da5 100644 --- a/R/html_dependency.R +++ b/R/html_dependency.R @@ -356,17 +356,10 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) { if (dependency$all_files) files <- list.files(dir) - else { - file_list <- c(dependency[['script']], - dependency[['stylesheet']], - dependency[['attachment']]) - if (anyUnnamed(file_list)) - files <- vapply(file_list, - function(f) if (is.list(f)) f[['src']] else f, - "") - else - files <- file_list[['src']] - } + else + files <- c(pluck_src(dependency$script), + pluck_src(dependency$stylesheet), + pluck_src(dependency$attachment)) if (length(files) == 0) { # This dependency doesn't include any files diff --git a/R/utils.R b/R/utils.R index c72ae1f1..493e0afc 100644 --- a/R/utils.R +++ b/R/utils.R @@ -151,3 +151,12 @@ anyUnnamed <- function(x) { # List with name attribute; check for any "" any(!nzchar(nms)) } + +# Get the source filename out of a script or similar +# entry. +pluck_src <- function(x) { + if (is.character(x)) return(x) + if (is.list(x) && "src" %in% names(x)) return(x$src) + if (is.list(x)) return(vapply(x, pluck_src, "")) + NULL +} diff --git a/tests/testthat/test-deps.r b/tests/testthat/test-deps.r index 85ba395b..b6337ef2 100644 --- a/tests/testthat/test-deps.r +++ b/tests/testthat/test-deps.r @@ -363,10 +363,13 @@ test_that("copyDependencyToDir() handles attributes", { dir.create(tmp_dep) on.exit(unlink(tmp_dep)) - tmp_txt <- basename(tempfile("text", fileext = ".txt")) - tmp_path <- file.path(tmp_dep, tmp_txt) - writeLines("Some text in the text/plain dep", tmp_path) - on.exit(unlink(tmp_path), add = TRUE) + tmp_txt <- "temp.txt" + path <- file.path(tmp_dep, tmp_txt) + writeLines("Some text in the text/plain dep", path) + + tmp_js <- "javascript.js" + path <- file.path(tmp_dep, tmp_js) + writeLines('console.log("log message");', path) tmp_rmd <- tempfile("rmd_files") dir.create(tmp_rmd) @@ -384,7 +387,7 @@ test_that("copyDependencyToDir() handles attributes", { name = "textPlain", version = "9.9.9", src = tmp_dep, - script = tmp_txt, + script = tmp_js, all_files = FALSE ) @@ -392,15 +395,52 @@ test_that("copyDependencyToDir() handles attributes", { name = "textPlain", version = "9.9.9", src = tmp_dep, - script = list(tmp_txt, + script = list(tmp_js, list(src = tmp_txt, type = "text/plain")), all_files = FALSE ) - # None of these should trigger errors as the first two did in - # issue #320 + + dep_with_one_nested <- htmltools::htmlDependency( + name = "textPlain", + version = "9.9.9", + src = tmp_dep, + script = list(list(src = tmp_txt, type = "text/plain")), + all_files = FALSE + ) + + dep_with_missings <- htmltools::htmlDependency( + name = "textPlain", + version = "9.9.9", + src = tmp_dep, + script = list(tmp_js, + "foobar1", + list(src = "foobar2")), + all_files = FALSE + ) + + # None of these except the last should trigger errors as + # the first two did in issue #320 copyDependencyToDir(dep_with_attributes, tmp_rmd) + expect_equal(dir(tmp_rmd, recursive = TRUE), + "textPlain-9.9.9/temp.txt") + + unlink(dir(tmp_rmd, recursive = TRUE)) copyDependencyToDir(dep_with_both, tmp_rmd) + expect_equal(dir(tmp_rmd, recursive = TRUE), + c("textPlain-9.9.9/javascript.js", + "textPlain-9.9.9/temp.txt")) + + unlink(dir(tmp_rmd, recursive = TRUE)) copyDependencyToDir(dep_without, tmp_rmd) - succeed() + expect_equal(dir(tmp_rmd, recursive = TRUE), + "textPlain-9.9.9/javascript.js") + + unlink(dir(tmp_rmd, recursive = TRUE)) + copyDependencyToDir(dep_with_one_nested, tmp_rmd) + expect_equal(dir(tmp_rmd, recursive = TRUE), + "textPlain-9.9.9/temp.txt") + + expect_error(copyDependencyToDir(dep_with_missings, tmp_rmd)) + }) From 01bd33849a13ee5796e098367f8ca5df4005be34 Mon Sep 17 00:00:00 2001 From: Winston Chang Date: Wed, 15 Jun 2022 15:37:19 -0500 Subject: [PATCH 5/5] Move <- out of 'if' statement --- R/html_dependency.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/R/html_dependency.R b/R/html_dependency.R index 99445da5..7b393ba8 100644 --- a/R/html_dependency.R +++ b/R/html_dependency.R @@ -369,11 +369,12 @@ copyDependencyToDir <- function(dependency, outputDir, mustWork = TRUE) { } srcfiles <- file.path(dir, files) - if (any(bad <- !file.exists(srcfiles))) { + missing_srcfiles <- !file.exists(srcfiles) + if (any(missing_srcfiles)) { stop( sprintf( "Can't copy dependency files that don't exist: '%s'", - paste(srcfiles[bad], collapse = "', '") + paste(srcfiles[missing_srcfiles], collapse = "', '") ) ) }