From b308dfb07e2df718a3057f8dcc27040a3ecea56a Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 2 Dec 2022 11:44:22 -0800 Subject: [PATCH 1/5] update comments for build_lesson --- R/build_lesson.R | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/R/build_lesson.R b/R/build_lesson.R index 5f399034f..b614c85d6 100644 --- a/R/build_lesson.R +++ b/R/build_lesson.R @@ -28,25 +28,43 @@ #' build_lesson(tmp) build_lesson <- function(path = ".", rebuild = FALSE, quiet = !interactive(), preview = TRUE, override = list()) { - # step 0: check pandoc installation; build_lesson defaults to a local build - check_pandoc() - # step 1: validate that the lesson structure makes sense + # Pre-flight compillation ---------------------------------------------------- + # + # In this part, we need to do a couple of things: + # 1. check that pandoc is present and fail early if it is not + check_pandoc() + # 2. check if we are only building one file and get its slug to pass to the + # markdown and site functions. slug <- if (fs::is_file(path)) get_slug(path) else NULL + # 3. set the source path global variable so that it can be used throughout the + # build process without explicitly needing to pass a variable from function + # to function, resetting the build path when the function exits (gracefully + # or ungracefully) path <- set_source_path(path) on.exit({ reset_build_paths() }) - # Validate the lesson and set the global values for the lesson. This includes + # 4. validate the lesson and set the global values for the lesson. + # This includes the following objects: # - # .store: the lesson as a pegboard::Lesson object - # .resources: a list of markdown resources for the lesson - # this_metadata: metadata with template for including in the pages - # learner_globals: variables for the learner version of the pages - # instructor_globals: variables for the instructor version of the pages + # - .store..............the lesson as a pegboard::Lesson object + # - .resources..........a list of markdown resources for the lesson + # - this_metadata.......metadata with template for including in the pages + # - learner_globals.....variables for the learner version of the pages + # - instructor_globals..variables for the instructor version of the pages validate_lesson(path, quiet = quiet) + # Building the markdown ------------------------------------------------------ + # + # Once we know we have all of the lesson components, we can build the markdown + # sources and store them in `site/built`. Only the markdown sources that have + # changed in content will be rebuilt with {knitr}. built <- build_markdown(path = path, rebuild = rebuild, quiet = quiet, slug = slug) + # Building the HTML ---------------------------------------------------------- + # + # This step uses the contents of `site/built` to build the website in + # `site/docs` with {whisker} and {pkgdown} build_site(path = path, quiet = quiet, preview = preview, override = override, slug = slug, built = built) } From 8825248b3e7631308ea7ff68da1a73e0f2c7aa44 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 2 Dec 2022 15:37:22 -0800 Subject: [PATCH 2/5] add describe process function This is sugar that replaces a cli call conditional --- R/utils-cli.R | 3 +++ 1 file changed, 3 insertions(+) diff --git a/R/utils-cli.R b/R/utils-cli.R index 76b5f9854..fa044b7d6 100644 --- a/R/utils-cli.R +++ b/R/utils-cli.R @@ -1,6 +1,9 @@ ci_group <- function(group = "Group") { cli::cat_line(glue::glue("::group::{group}")) } +describe_progress <- function(..., quiet = FALSE) { + if (!quiet) cli::cli_rule(cli::style_bold(...)) +} sandpaper_cli_theme <- function() { list( From 796f982c6819e12decbe427afa686c4c7f9ebdb1 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 2 Dec 2022 15:38:35 -0800 Subject: [PATCH 3/5] update shim to process both functions on exit --- R/build_site.R | 2 +- inst/pkgdown/shim.R | 6 ++++-- tests/testthat/test-build_html.R | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/R/build_site.R b/R/build_site.R index 60314a7a0..5a9affeb3 100644 --- a/R/build_site.R +++ b/R/build_site.R @@ -65,7 +65,7 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr names(pct) <- db$file[er[1]:er[2]] # shim for downlit ---------------------------------------------------------- shimstem_file <- system.file("pkgdown", "shim.R", package = "sandpaper") - expected <- "41aea9a01589d636768f56a333343ec5" + expected <- "62a291ba3386aad91e5ade20480ad7cf" actual <- tools::md5sum(shimstem_file) if (expected == actual) { # evaluate the shim in our namespace diff --git a/inst/pkgdown/shim.R b/inst/pkgdown/shim.R index ca7a3b367..a9a6ab7df 100644 --- a/inst/pkgdown/shim.R +++ b/inst/pkgdown/shim.R @@ -18,5 +18,7 @@ unlockBinding("token_href", dl) unlockBinding("href_topic", dl) dl$token_href <- function(token, ...) rep(NA, length(token)) dl$href_topic <- function(...) NA_character_ -parse(text = "{\ndl$token_href <- tr\nlockBinding('token_href', dl)\n}") -parse(text = "{\ndl$href_topic <- ht\nlockBinding('href_topic', dl)\n}") +c( + parse(text = "{\ndl$token_href <- tr\nlockBinding('token_href', dl)\n}"), + parse(text = "{\ndl$href_topic <- ht\nlockBinding('href_topic', dl)\n}") +) diff --git a/tests/testthat/test-build_html.R b/tests/testthat/test-build_html.R index 924ada094..0a8e69a92 100644 --- a/tests/testthat/test-build_html.R +++ b/tests/testthat/test-build_html.R @@ -7,12 +7,14 @@ set_globals(res) pkg <- pkgdown::as_pkgdown(path_site(res)) # shim for downlit ---------------------------------------------------------- shimstem_file <- system.file("pkgdown", "shim.R", package = "sandpaper") -expected <- "5484c37e9b9c324361d775a10dea4946" +expected <- "62a291ba3386aad91e5ade20480ad7cf" actual <- tools::md5sum(shimstem_file) if (expected == actual) { # evaluate the shim in our namespace when_done <- source(shimstem_file, local = TRUE)$value withr::defer(eval(when_done)) +} else { + stop("shim broken") } # end downlit shim ---------------------------------------------------------- @@ -28,7 +30,7 @@ test_that("[build_home()] works independently", { fs::dir_create(built_dir) fs::file_copy(fs::path(res, "index.md"), built_dir) fs::file_copy(fs::path(res, "learners", "setup.md"), built_dir) - build_home(pkg, quiet = TRUE, new_setup = TRUE, + build_home(pkg, quiet = TRUE, next_page = fs::path(res, "episodes", "introduction.Rmd") ) learn_index <- fs::path(pkg$dst_path, "index.html") From d62446286c6632902848e4eb29ca4679045380cd Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 2 Dec 2022 15:40:36 -0800 Subject: [PATCH 4/5] update build_site docs; remove unused args build_home and build_profiles no longer need the schedule passed to them, so we no longer need to build it in build_site as it is provided by this_lesson() --- NEWS.md | 5 +++ R/build_home.R | 2 +- R/build_profiles.R | 2 +- R/build_site.R | 87 +++++++++++++++++++++++++++------------------- 4 files changed, 59 insertions(+), 37 deletions(-) diff --git a/NEWS.md b/NEWS.md index 25ec6d82f..dfc7a445f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,8 @@ +# sandpaper 0.11.1 + +* code inside of main `build_` functions cleaned up. Unused variables removed + and narrative comments added to provide context. + # sandpaper 0.11.0 * Documentation for internal storage objects updated. diff --git a/R/build_home.R b/R/build_home.R index 8ce47e179..f19fa8433 100644 --- a/R/build_home.R +++ b/R/build_home.R @@ -1,4 +1,4 @@ -build_home <- function(pkg, quiet, sidebar = NULL, new_setup = TRUE, next_page = NULL) { +build_home <- function(pkg, quiet, next_page = NULL) { page_globals <- setup_page_globals() path <- root_path(pkg$src_path) syl <- format_syllabus(get_syllabus(path, questions = TRUE), use_col = FALSE) diff --git a/R/build_profiles.R b/R/build_profiles.R index fa197991e..a28ece773 100644 --- a/R/build_profiles.R +++ b/R/build_profiles.R @@ -1,4 +1,4 @@ -build_profiles <- function(pkg, quiet, sidebar = NULL) { +build_profiles <- function(pkg, quiet) { page_globals <- setup_page_globals() path <- root_path(pkg$src_path) profs <- get_profiles(path, trim = FALSE) diff --git a/R/build_site.R b/R/build_site.R index 5a9affeb3..431bf0498 100644 --- a/R/build_site.R +++ b/R/build_site.R @@ -11,13 +11,24 @@ #' @param built a character vector of newly built files or NULL. #' @keywords internal build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, override = list(), slug = NULL, built = NULL) { - # step 1: check pandoc + # Setup ---------------------------------------------------------------------- + # + # Because this can be run independently of build_lesson(), we need to check + # that pandoc exists and to provision the global lesson components if they do + # not yet exist. check_pandoc(quiet) this_lesson(path) + # One feature of The Workbench is a global common links file that will be + # appended to the markdown files before they are sent to be rendered into + # HTML so that they will render the links correctly. cl <- getOption("sandpaper.links") on.exit(options(sandpaper.links = cl), add = TRUE) set_common_links(path) - # step 2: build the package site + + # Initialise Site ------------------------------------------------------------ + # + # Here we provision our website using pkgdown and either initialise it if it + # does not exist or update the CSS, HTML, and JS if it does exist. pkg <- pkgdown::as_pkgdown(path_site(path), override = override) built_path <- fs::path(pkg$src_path, "built") # NOTE: This is a kludge to prevent pkgdown from displaying a bunch of noise @@ -32,22 +43,24 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr } pkgdown::init_site(pkg) fs::file_create(fs::path(pkg$dst_path, ".nojekyll")) - # future plans to reduce build times + # NOTE: future plans to reduce build times rebuild_template <- TRUE || !template_check$valid() - new_setup <- any(grepl("[/]setup[.]md", built)) + # Determining what to rebuild ------------------------------------------------ + describe_progress("Scanning episodes to rebuild", quiet = quiet) + # + # The 'built' object is a character vector with files that have been rebuilt. + # new_setup <- any(grepl("[/]setup[.]md", built)) db <- get_built_db(fs::path(built_path, "md5sum.txt")) # filter out files that we will combine to generate db <- reserved_db(db) # Find all the episodes and get their range er <- range(grep("episodes/", db$file, fixed = TRUE)) - - # Absolute paths for pandoc - abs_md <- fs::path(path, db$built) + # Get absolute paths for pandoc to understand + abs_md <- fs::path(path, db$built) abs_src <- fs::path(path, db$file) - - if (!quiet) cli::cli_rule(cli::style_bold("Scanning episodes to rebuild")) - + chapters <- abs_md[seq(er[1], er[2])] + # If we are only processing one file, then the output should be that one file if (is.null(slug)) { out <- "index.html" files_to_render <- seq_along(db$built) @@ -55,36 +68,35 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr out <- paste0(slug, ".html") files_to_render <- which(get_slug(db$built) == slug) } - - out <- if (is.null(slug)) "index.html" else paste0(slug, ".html") - chapters <- abs_md[seq(er[1], er[2])] - sidebar <- create_sidebar(c(fs::path(built_path, "index.md"), chapters)) - + + # Rebuilding Episodes and generated files ------------------------------------ # Get percentages from the syllabus table pct <- get_syllabus(path, questions = TRUE)$percents names(pct) <- db$file[er[1]:er[2]] - # shim for downlit ---------------------------------------------------------- + # ------------------------ shim for downlit ---------------------------- + # Bypass certain downlit functions that produce unintented effects such + # as linking function documentation. shimstem_file <- system.file("pkgdown", "shim.R", package = "sandpaper") expected <- "62a291ba3386aad91e5ade20480ad7cf" actual <- tools::md5sum(shimstem_file) if (expected == actual) { # evaluate the shim in our namespace when_done <- source(shimstem_file, local = TRUE)$value + # restore the original functions on exit on.exit(eval(when_done), add = TRUE) } - # end downlit shim ---------------------------------------------------------- + # ------------------------ end downlit shim ---------------------------- for (i in files_to_render) { location <- page_location(i, abs_md, er) build_episode_html( - path_md = abs_md[i], - path_src = abs_src[i], - page_back = location["back"], - page_forward = location["forward"], + path_md = abs_md[i], + path_src = abs_src[i], + page_back = location["back"], + page_forward = location["forward"], page_progress = pct[db$file[i]], - sidebar = sidebar, - date = db$date[i], - pkg = pkg, - quiet = quiet + date = db$date[i], + pkg = pkg, + quiet = quiet ) } # if (rebuild_template) template_check$set() @@ -92,12 +104,17 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr fs::dir_walk(built_path, function(d) copy_assets(d, pkg$dst_path), all = TRUE) - if (!quiet) cli::cli_rule(cli::style_bold("Creating learner profiles")) - build_profiles(pkg, quiet = quiet, sidebar = sidebar) - if (!quiet) cli::cli_rule(cli::style_bold("Creating homepage")) - build_home(pkg, quiet = quiet, sidebar = sidebar, new_setup = new_setup, - next_page = abs_md[er[1]] - ) + # Combined pages ------------------------------------------------------------- + # + # There are two pages that are the result of source file combinations: + # + # 1. learner profiles which concatenates the files in the profiles/ folder + describe_progress("Creating learner profiles", quiet = quiet) + build_profiles(pkg, quiet = quiet)#, sidebar = sidebar) + # + # 2. home page which concatenates index.md and learners/setup.md + describe_progress("Creating homepage", quiet = quiet) + build_home(pkg, quiet = quiet, next_page = abs_md[er[1]]) # Generated content ---------------------------------------------------------- # @@ -121,13 +138,13 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr # Once we have the pre-processed templates and HTML content, we can pass these # to our aggregator functions: - if (!quiet) cli::cli_rule(cli::style_bold("Creating keypoints summary")) + describe_progress("Creating keypoints summary", quiet = quiet) build_keypoints(pkg, pages = html_pages, quiet = quiet) - if (!quiet) cli::cli_rule(cli::style_bold("Creating All-in-one page")) + describe_progress("Creating All-in-one page", quiet = quiet) build_aio(pkg, pages = html_pages, quiet = quiet) - if (!quiet) cli::cli_rule(cli::style_bold("Creating Images page")) + describe_progress("Creating Images page", quiet = quiet) build_images(pkg, pages = html_pages, quiet = quiet) - if (!quiet) cli::cli_rule(cli::style_bold("Creating Instructor Notes")) + describe_progress("Creating Instructor Notes", quiet = quiet) build_instructor_notes(pkg, pages = html_pages, built = built, quiet = quiet) # At the end, a sitemap is created with our aggregated pages. From e24c860c7540fb325371724f2f91f564290e321a Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Mon, 5 Dec 2022 07:01:11 -0800 Subject: [PATCH 5/5] hard-code site path for silly bus This is a hotfix for #371 --- DESCRIPTION | 2 +- NEWS.md | 10 ++++++++++ R/get_syllabus.R | 3 ++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index ea3461070..051417037 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: sandpaper Title: Create and Curate Carpentries Lessons -Version: 0.11.0 +Version: 0.11.1 Authors@R: c( person(given = "Zhian N.", family = "Kamvar", diff --git a/NEWS.md b/NEWS.md index dfc7a445f..f1d35ef3b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,15 @@ # sandpaper 0.11.1 +BUG FIX +------- + +* A bug in deployment where GitHub deployments fail due to an error that says + "Error: Can't find DESCRIPTION" has been fixed (reported: @tobyhodges, #371; + fixed, @zkamvar, #372) + +MISC +---- + * code inside of main `build_` functions cleaned up. Unused variables removed and narrative comments added to provide context. diff --git a/R/get_syllabus.R b/R/get_syllabus.R index 01edf534e..54f311e4c 100644 --- a/R/get_syllabus.R +++ b/R/get_syllabus.R @@ -45,7 +45,8 @@ create_syllabus <- function(episodes, lesson, path, questions = TRUE) { timings <- vapply(episodes, get_timings, numeric(1)) titles <- vapply(episodes, get_titles, character(1)) - paths <- fs::path(pkgdown::as_pkgdown(path_site(path))$dst_path, sched) + # pkgdown::as_pkgdown + paths <- fs::path(path_site(path), "docs", sched) paths <- fs::path_ext_set(paths, "html") start <- as.POSIXlt("00:00", format = "%H:%M", tz = "UTC")