Merge pull request #372 from carpentries/update-overall-docs

Hotfix: do not invoke pkgdown before site/ is provisioned on GitHub; documentation updates

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",

NEWS.md

@@ -1,3 +1,18 @@
+# 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. 
+
 # sandpaper 0.11.0
 
 * Documentation for internal storage objects updated.

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)

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)
 
 } 

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)

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,72 +43,78 @@ 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)
   } else {
     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 <- "41aea9a01589d636768f56a333343ec5"
+  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()
 
   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.

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")

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(

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}")
+)

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")