From f6060edc69a0e6185f85928fdc649fcba6e64388 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 24 Apr 2024 08:19:18 -0500 Subject: [PATCH] Revamp messaging when copying files (#2492) * Once again show one file per line * Make it possible to nicely label special roots (i.e. the pkgdown package) --- NEWS.md | 1 + R/build-favicons.R | 11 ++-- R/build-logo.R | 7 +- R/build-reference.R | 22 ++++--- R/init.R | 58 +++++++++-------- R/package.R | 7 +- R/templates.R | 2 +- R/theme.R | 6 +- R/utils-fs.R | 86 +++++++++++++------------ tests/testthat/_snaps/build-articles.md | 3 +- tests/testthat/_snaps/build-logo.md | 3 +- tests/testthat/_snaps/init.md | 15 +++++ tests/testthat/test-init.R | 5 ++ 13 files changed, 125 insertions(+), 101 deletions(-) diff --git a/NEWS.md b/NEWS.md index b582dc5daa..7552623262 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # pkgdown (development version) +* `init_site()` once again describes one copy per line, and now uses a better prefix when copying assets from pkgdown itself (#2445). * Very wide words are now automatically broken across lines and hyphenated (when possible) when they'd otherwise create a horizontal scrollbar on mobile (#1888). * The `repo.source.url` field no longer requires a trailing slash (#2017). * Anywhere you can use `_pkgdown.yml`, you can now use `_pkgdown.yaml` (#2244). diff --git a/R/build-favicons.R b/R/build-favicons.R index 27c425d2a1..76ae0623fd 100644 --- a/R/build-favicons.R +++ b/R/build-favicons.R @@ -109,11 +109,12 @@ build_favicons <- function(pkg = ".", overwrite = FALSE) { copy_favicons <- function(pkg = ".") { pkg <- as_pkgdown(pkg) - favicons <- path(pkg$src_path, "pkgdown", "favicon") - if (!dir_exists(favicons)) - return() - - dir_copy_to(pkg, favicons, pkg$dst_path) + dir_copy_to( + src_dir = path(pkg$src_path, "pkgdown", "favicon"), + src_root = pkg$src_path, + dst_dir = pkg$dst_path, + dst_root = pkg$dst_path + ) } has_favicons <- function(pkg = ".") { diff --git a/R/build-logo.R b/R/build-logo.R index 94241a44be..80b8d4d732 100644 --- a/R/build-logo.R +++ b/R/build-logo.R @@ -3,7 +3,12 @@ copy_logo <- function(pkg = ".") { logo_path <- find_logo(pkg$src_path) if (!is.null(logo_path)) { - file_copy_to(pkg, logo_path, from_dir = path_dir(logo_path)) + file_copy_to( + src_paths = logo_path, + src_root = pkg$src_path, + dst_paths = path(pkg$dst_path, path_file(logo_path)), + dst_root = pkg$dst_path + ) } } diff --git a/R/build-reference.R b/R/build-reference.R index 4d8c3cc860..46c158dac2 100644 --- a/R/build-reference.R +++ b/R/build-reference.R @@ -207,11 +207,12 @@ build_reference <- function(pkg = ".", copy_figures <- function(pkg) { # copy everything from man/figures to docs/reference/figures - src_figures <- path(pkg$src_path, "man", "figures") - dst_figures <- path(pkg$dst_path, "reference", "figures") - if (file_exists(src_figures)) { - dir_copy_to(pkg, src_figures, dst_figures) - } + dir_copy_to( + src_dir = path(pkg$src_path, "man", "figures"), + src_root = pkg$src_path, + dst_dir = path(pkg$dst_path, "reference", "figures"), + dst_root = pkg$dst_path + ) } examples_env <- function(pkg, seed = 1014L, devel = TRUE, envir = parent.frame()) { @@ -254,11 +255,12 @@ build_reference_index <- function(pkg = ".") { create_subdir(pkg, "reference") # Copy icons, if needed - src_icons <- path(pkg$src_path, "icons") - dst_icons <- path(pkg$dst_path, "reference", "icons") - if (file_exists(src_icons)) { - dir_copy_to(pkg, src_icons, dst_icons) - } + dir_copy_to( + src_dir = path(pkg$src_path, "icons"), + src_root = pkg$src_path, + dst_dir = path(pkg$dst_path, "reference", "icons"), + dst_root = pkg$dst_path + ) render_page( pkg, "reference-index", diff --git a/R/init.R b/R/init.R index 31d2c9a413..ccbba6bae4 100644 --- a/R/init.R +++ b/R/init.R @@ -58,23 +58,22 @@ copy_assets <- function(pkg = ".") { # pkgdown assets if (!identical(template$default_assets, FALSE)) { - copy_asset_dir(pkg, path_pkgdown(paste0("BS", pkg$bs_version), "assets")) - } - - # manually specified directory: I don't think this is documented - # and no longer seems important, so I suspect it could be removed - if (!is.null(template$assets)) { - copy_asset_dir(pkg, template$assets) + copy_asset_dir( + pkg, + path_pkgdown(paste0("BS", pkg$bs_version, "/", "assets")), + src_root = path_pkgdown(), + src_label = "/" + ) } # package assets if (!is.null(template$package)) { - assets <- path_package_pkgdown( - "assets", - package = template$package, - bs_version = pkg$bs_version + copy_asset_dir( + pkg, + path_package_pkgdown("assets", template$package, pkg$bs_version), + src_root = system_file(package = template_package), + src_label = paste0("<", template$package, ">/") ) - copy_asset_dir(pkg, assets) } # extras @@ -85,27 +84,32 @@ copy_assets <- function(pkg = ".") { invisible() } -copy_asset_dir <- function(pkg, from_dir, file_regexp = NULL) { - if (length(from_dir) == 0) { - return(character()) - } - from_path <- path_abs(from_dir, pkg$src_path) - if (!file_exists(from_path)) { +copy_asset_dir <- function(pkg, + dir, + src_root = pkg$src_path, + src_label = "", + file_regexp = NULL) { + src_dir <- path_abs(dir, pkg$src_path) + if (!file_exists(src_dir)) { return(character()) } - files <- dir_ls(from_path, recurse = TRUE) - - # Remove directories from files - files <- files[!fs::is_dir(files)] - + src_paths <- dir_ls(src_dir, recurse = TRUE) + src_paths <- src_paths[!fs::is_dir(src_paths)] if (!is.null(file_regexp)) { - files <- files[grepl(file_regexp, path_file(files))] + src_paths <- src_paths[grepl(file_regexp, path_file(src_paths))] } - # Handled in bs_theme() - files <- files[path_ext(files) != "scss"] + src_paths <- src_paths[path_ext(src_paths) != "scss"] # Handled in bs_theme() - file_copy_to(pkg, files, pkg$dst_path, from_dir = from_path) + dst_paths <- path(pkg$dst_path, path_rel(src_paths, src_dir)) + + file_copy_to( + src_paths = src_paths, + src_root = src_root, + src_label = src_label, + dst_paths = dst_paths, + dst_root = pkg$dst_path + ) } timestamp <- function(time = Sys.time()) { diff --git a/R/package.R b/R/package.R index ed950b96e8..cbef499c51 100644 --- a/R/package.R +++ b/R/package.R @@ -336,12 +336,7 @@ find_template_config <- function(package, bs_version = NULL) { return(list()) } - config <- path_package_pkgdown( - "_pkgdown.yml", - package = package, - bs_version = bs_version - ) - + config <- path_package_pkgdown("_pkgdown.yml", package, bs_version) if (!file_exists(config)) { return(list()) } diff --git a/R/templates.R b/R/templates.R index 54816e4e50..4f51ff822b 100644 --- a/R/templates.R +++ b/R/templates.R @@ -54,7 +54,7 @@ templates_dir <- function(pkg = list()) { } path_abs(template$path, start = pkg$src_path) } else if (!is.null(template$package)) { - path_package_pkgdown("templates", package = template$package, bs_version = pkg$bs_version) + path_package_pkgdown("templates", template$package, pkg$bs_version) } else { path(pkg$src_path, "pkgdown", "templates") } diff --git a/R/theme.R b/R/theme.R index d3f188584e..265b5082e1 100644 --- a/R/theme.R +++ b/R/theme.R @@ -68,11 +68,7 @@ bs_theme_rules <- function(pkg) { package <- purrr::pluck(pkg, "meta", "template", "package") if (!is.null(package)) { - package_extra <- path_package_pkgdown( - "extra.scss", - package = package, - bs_version = pkg$bs_version - ) + package_extra <- path_package_pkgdown("extra.scss", package, pkg$bs_version) if (file_exists(package_extra)) { paths <- c(paths, package_extra) } diff --git a/R/utils-fs.R b/R/utils-fs.R index 20b669fc66..a48bfedd81 100644 --- a/R/utils-fs.R +++ b/R/utils-fs.R @@ -1,53 +1,55 @@ -dir_copy_to <- function(pkg, from, to, overwrite = TRUE) { - stopifnot(length(to) == 1) - new_path <- function(path) { - path_abs(path_rel(path, start = from), start = to) +dir_copy_to <- function(src_dir, + dst_dir, + src_root, + dst_root, + src_label = "", + dst_label = "") { + check_string(src_dir) + check_string(dst_dir) + + if (!dir_exists(src_dir)) { + return() } - contents <- dir_ls(from, recurse = TRUE) - is_dir <- fs::is_dir(contents) + src_paths <- dir_ls(src_dir, recurse = TRUE) + is_dir <- fs::is_dir(src_paths) + + dst_paths <- path(dst_dir, path_rel(src_paths, src_dir)) # First create directories - dir_create(to) - dirs <- contents[is_dir] - dir_create(new_path(dirs)) - + dir_create(dst_paths[is_dir]) # Then copy files - file_copy_to(pkg, contents[!is_dir], - to_dir = to, - from_dir = from, - overwrite = overwrite + file_copy_to( + src_paths = src_paths[!is_dir], + dst_paths = dst_paths[!is_dir], + src_root = src_root, + dst_root = dst_root, + src_label = src_label, + dst_label = dst_label ) } -# Would be better to base on top of data structure that provides both -# files and root directory to use for printing -file_copy_to <- function(pkg, - from_paths, - to_dir = pkg$dst_path, - from_dir = path_common(from_paths), - overwrite = TRUE) { - - if (length(from_paths) == 0) { - return() - } - - from_rel <- path_rel(from_paths, from_dir) - to_paths <- path_abs(from_rel, to_dir) - +file_copy_to <- function(src_paths, + dst_paths, + src_root, + dst_root, + src_label = "", + dst_label = "") { # Ensure all the "to" directories exist - dirs_to_paths <- unique(fs::path_dir(to_paths)) - dir_create(dirs_to_paths) + dst_dirs <- unique(fs::path_dir(dst_paths)) + dir_create(dst_dirs) - eq <- purrr::map2_lgl(from_paths, to_paths, file_equal) + eq <- purrr::map2_lgl(src_paths, dst_paths, file_equal) if (any(!eq)) { - cli::cli_inform(c( - "Copying {src_path(path_rel(from_paths[!eq], pkg$src_path))}", - " to {dst_path(path_rel(to_paths[!eq], pkg$dst_path))}" - )) + src <- paste0(src_label, path_rel(src_paths[!eq], src_root)) + dst <- paste0(dst_label, path_rel(dst_paths[!eq], dst_root)) + + purrr::walk2(src, dst, function(src, dst) { + cli::cli_inform("Copying {src_path(src)} to {dst_path(dst)}") + }) } - file_copy(from_paths[!eq], to_paths[!eq], overwrite = overwrite) + file_copy(src_paths[!eq], dst_paths[!eq], overwrite = TRUE) } # Checks init_site() first. @@ -94,19 +96,19 @@ path_first_existing <- function(...) { NULL } -path_package_pkgdown <- function(..., package, bs_version = NULL) { +path_package_pkgdown <- function(path, package, bs_version) { check_installed(package) base <- system_file("pkgdown", package = package) # If bs_version supplied, first try for versioned template if (!is.null(bs_version)) { - path <- path(base, paste0("BS", bs_version), ...) - if (file_exists(path)) { - return(path) + ver_path <- path(base, paste0("BS", bs_version), path) + if (file_exists(ver_path)) { + return(ver_path) } } - path(base, ...) + path(base, path) } path_pkgdown <- function(...) { diff --git a/tests/testthat/_snaps/build-articles.md b/tests/testthat/_snaps/build-articles.md index f64bcaa2ab..30f177da67 100644 --- a/tests/testthat/_snaps/build-articles.md +++ b/tests/testthat/_snaps/build-articles.md @@ -3,8 +3,7 @@ Code copy_figures(pkg) Message - Copying man/figures/kitten.jpg - to reference/figures/kitten.jpg + Copying man/figures/kitten.jpg to reference/figures/kitten.jpg --- diff --git a/tests/testthat/_snaps/build-logo.md b/tests/testthat/_snaps/build-logo.md index bf005edad5..60cf0e1d85 100644 --- a/tests/testthat/_snaps/build-logo.md +++ b/tests/testthat/_snaps/build-logo.md @@ -3,6 +3,5 @@ Code copy_logo(pkg) Message - Copying man/figures/logo.svg - to logo.svg + Copying man/figures/logo.svg to logo.svg diff --git a/tests/testthat/_snaps/init.md b/tests/testthat/_snaps/init.md index 953b138c4b..ffc67baba3 100644 --- a/tests/testthat/_snaps/init.md +++ b/tests/testthat/_snaps/init.md @@ -1,3 +1,18 @@ +# informative print method + + Code + init_site(pkg) + Message + -- Initialising site ----------------------------------------------------------- + Copying /BS3/assets/bootstrap-toc.css to bootstrap-toc.css + Copying /BS3/assets/bootstrap-toc.js to bootstrap-toc.js + Copying /BS3/assets/docsearch.css to docsearch.css + Copying /BS3/assets/docsearch.js to docsearch.js + Copying /BS3/assets/link.svg to link.svg + Copying /BS3/assets/pkgdown.css to pkgdown.css + Copying /BS3/assets/pkgdown.js to pkgdown.js + Copying pkgdown/extra.css to extra.css + # site meta doesn't break unexpectedly Code diff --git a/tests/testthat/test-init.R b/tests/testthat/test-init.R index 65d8c34372..27e6d2e275 100644 --- a/tests/testthat/test-init.R +++ b/tests/testthat/test-init.R @@ -1,3 +1,8 @@ +test_that("informative print method", { + pkg <- local_pkgdown_site(test_path("assets/init-extra-1")) + expect_snapshot(init_site(pkg)) +}) + test_that("extra.css and extra.js copied and linked", { pkg <- local_pkgdown_site(test_path("assets/init-extra-2")) suppressMessages(init_site(pkg))