From bdb6ac3282c05608886309fd08e5f5face501589 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 16 Nov 2023 11:20:09 -0800 Subject: [PATCH 1/6] add switch to differentiate images and links This will address #143 --- R/utils-validation.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/utils-validation.R b/R/utils-validation.R index ce57ce30..5c0f4403 100644 --- a/R/utils-validation.R +++ b/R/utils-validation.R @@ -55,7 +55,8 @@ throw_link_warnings <- function(VAL) { reports <- line_report(msg = err$labels, err$filepath, err$sourcepos, sep = " ") failed <- !apply(err[names(link_tests)], MARGIN = 2, all) - issue_warning(what = "links", + types <- paste0(unique(sub("img", "image", err$type)), "s") + issue_warning(what = paste(types, collapse = " and "), cli = has_cli(), n = nrow(err), N = nrow(VAL), From 865ad2e645c411cf4e065e7a6ba5c6e5c7f02d5b Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 16 Nov 2023 11:21:29 -0800 Subject: [PATCH 2/6] document and rerun tests --- NAMESPACE | 1 + NEWS.md | 5 ++ R/utils-cli.R | 8 ++- R/utils-validation.R | 68 +++++++++++++++++++++ man/cli_helpers.Rd | 8 ++- man/make_link_table.Rd | 5 +- man/throw_warnings.Rd | 91 +++++++++++++++++++++++++++++ tests/testthat/_snaps/Lesson.md | 8 +-- tests/testthat/_snaps/validation.md | 12 ++-- 9 files changed, 189 insertions(+), 17 deletions(-) create mode 100644 man/throw_warnings.Rd diff --git a/NAMESPACE b/NAMESPACE index a9860836..68541af4 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -16,6 +16,7 @@ export(lesson_fragment) export(link_tests) export(liquid_to_commonmark) export(make_link_table) +export(throw_heading_warnings) importFrom(R6,R6Class) importFrom(glue,glue) importFrom(purrr,map) diff --git a/NEWS.md b/NEWS.md index 4fcb9cbe..183410a6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,11 @@ * Snapshot update for testthat 3.2.0 +## BUGFIX + +* `validate_links()` now differentiates between links and images in reporting + (reported: @joelnitta, #143; fixed: @zkamvar, #144) + # pegboard 0.7.1 (2023-10-03) * child chunk options that would fail out of context no longer cause a failure. diff --git a/R/utils-cli.R b/R/utils-cli.R index 241b4817..b45461ea 100644 --- a/R/utils-cli.R +++ b/R/utils-cli.R @@ -98,8 +98,10 @@ pb_message <- function (..., domain = NULL, appendLF = TRUE) { #' #' @param path path to the file to report #' @param pos position of the error -#' @param type the type of warning that should be thrown (defaults to warning) -#' @param sep a character to use to separate the human message and the line number +#' @param type (used in the context of CI only) the type of warning that should +#' be thrown (defaults to warning) +#' @param sep a character to use to separate the human message and the line +#' number #' @rdname cli_helpers line_report <- function(msg = "", path, pos, sep = "\t", type = "warning") { ci <- Sys.getenv("CI") != "" @@ -130,7 +132,7 @@ line_report <- function(msg = "", path, pos, sep = "\t", type = "warning") { #' cli = requireNamespace("cli", quietly = TRUE), #' f = "col_cyan" #' ) -#' cat(glue::glue("[{x}]->[{x2}]")) +#' writeLines(glue::glue("[{x}]->[{x2}]")) append_labels <- function(l, i = TRUE, e = "", cli = FALSE, f = "style_inverse") { f <- if (cli) utils::getFromNamespace(f, "cli") else function(e) e l[i] <- paste(l[i], f(e)) diff --git a/R/utils-validation.R b/R/utils-validation.R index 5c0f4403..985fd2c2 100644 --- a/R/utils-validation.R +++ b/R/utils-validation.R @@ -1,3 +1,69 @@ +#' Throw a validation report as a single message +#' +#' Collapse a variable number of validation reports into a single message that +#' can be formatted for the CLI or GitHub. +#' +#' @param VAL `[data.frame]` a validation report derived from one of the +#' `validate` functions. +#' @return NULL, invisibly. This is used for it's side-effect of formatting and +#' issuing messages via [issue_warning()]. +#' @export +#' @details One of the key features of {pegboard} is the ability to parse and +#' validate markdown elements. These functions provide a standard way of +#' creating the reports that are for the user based on whether or not they are +#' on the CLI or on GitHub. The prerequisites of these functions are the input +#' data frame (generated from the actual validation function) and an internal +#' set of known templating vectors that contain templates for each test to show +#' the actual error along with general information that can help correct the +#' error (see below). +#' +#' +#' ## Input Data Frame +#' +#' The validations are initially reported in a data frame that has the +#' following properties: +#' - one row per element +#' - columns that indicate the parsed attributes of the element, source +#' file, source position, and the actual element XML node object. +#' - boolean columns that indicate the tests for each element, used with +#' [collect_labels()] to add a "labels" column to the data. +#' +#' ## Templating vectors +#' +#' These vectors come in two forms `[thing]_tests` and `[thing]_info` (e.g. +#' for [validate_links()], we have `link_tests` and `link_info`). These are +#' named vectors that match the boolean columns of the data frame produced +#' by the validation function. The `[thing]_tests` vector contains templates +#' that describes the error and shows the text that caused the error. The +#' `[thing]_info` contains general information about how to address that +#' particular error. For example, one common link error is that a link is not +#' descriptive (e.g. the link text says "click here"). The column in the `VAL` +#' data frame that contains the result of this test is called "descriptive", so +#' if we look at the values from the link info and tests vectors: +#' +#' ```{r} +#' link_info["descriptive"] +#' link_tests["descriptive"] +#' ``` +#' +#' If the `throw_*_warnings()` functions detect any errors, they will use the +#' info and tests vectors to construct a composite message. +#' +#' ## Process +#' +#' The `throw_*_warnings()` functions all do the same basic procedure (and +#' indeed could be consolidated into a single function in the future) +#' +#' 1. pass data to [collect_labels()], which will parse the `[thing]_tests` +#' templating vector and label each failing element in `VAL` with the +#' appropriate failure message +#' 2. gather the source information for each failure +#' 3. pass failures with the `[thing]_info` elements that matched the unique +#' failures to [issue_warning()] +#' @seealso +#' [validate_links()], [validate_divs()], and [validate_headings()] for +#' input sources for these functions. +#' @rdname throw_warnings throw_heading_warnings <- function(VAL) { if (length(VAL) == 0 || nrow(VAL) == 0) { return(invisible(NULL)) @@ -20,6 +86,7 @@ throw_heading_warnings <- function(VAL) { reports = reports) } +#' @rdname throw_warnings throw_div_warnings <- function(VAL) { if (length(VAL) == 0 || nrow(VAL) == 0) { return(invisible(NULL)) @@ -41,6 +108,7 @@ throw_div_warnings <- function(VAL) { reports = reports) } +#' @rdname throw_warnings throw_link_warnings <- function(VAL) { if (length(VAL) == 0 || nrow(VAL) == 0) { return(invisible(NULL)) diff --git a/man/cli_helpers.Rd b/man/cli_helpers.Rd index 6f504e8e..4b3219e1 100644 --- a/man/cli_helpers.Rd +++ b/man/cli_helpers.Rd @@ -57,9 +57,11 @@ message_muffler(expr, keep = FALSE) \item{pos}{position of the error} -\item{sep}{a character to use to separate the human message and the line number} +\item{sep}{a character to use to separate the human message and the line +number} -\item{type}{the type of warning that should be thrown (defaults to warning)} +\item{type}{(used in the context of CI only) the type of warning that should +be thrown (defaults to warning)} \item{l}{a vector/list of characters} @@ -99,7 +101,7 @@ x2 <- pegboard:::append_labels(x, cli = requireNamespace("cli", quietly = TRUE), f = "col_cyan" ) -cat(glue::glue("[{x}]->[{x2}]")) +writeLines(glue::glue("[{x}]->[{x2}]")) pegboard:::message_muffler({ cli::cli_text("hello there! I'm staying in!") pegboard:::pb_message("normal looking message that's not getting through") diff --git a/man/make_link_table.Rd b/man/make_link_table.Rd index cd8c37f4..a40de259 100644 --- a/man/make_link_table.Rd +++ b/man/make_link_table.Rd @@ -16,7 +16,7 @@ a data frame containing the following columns: \item server The first part of the URL (e.g. doi.org or github.com) \item port the port number if it exists (note: liquid tags produce weird ports) \item user associated with port, usually blank -\item path path to the page in question +\item path the path element of the link \item query anything after a "?" in a URL \item fragment navigation within a page; anything after "#" in a URL \item orig the original, unparsed URL @@ -26,6 +26,9 @@ a data frame containing the following columns: \item rel if it's a relative URL, the name of the anchor, otherwise NA. \item anchor logical if the URL is an anchor \item sourcepos the source position in the file +\item filepath relative path to the source file +\item parents list column of paths to the build parents +\item node a list column of the nodes with the links } } \description{ diff --git a/man/throw_warnings.Rd b/man/throw_warnings.Rd new file mode 100644 index 00000000..5c6b7044 --- /dev/null +++ b/man/throw_warnings.Rd @@ -0,0 +1,91 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/utils-validation.R +\name{throw_heading_warnings} +\alias{throw_heading_warnings} +\alias{throw_div_warnings} +\alias{throw_link_warnings} +\title{Throw a validation report as a single message} +\usage{ +throw_heading_warnings(VAL) + +throw_div_warnings(VAL) + +throw_link_warnings(VAL) +} +\arguments{ +\item{VAL}{\verb{[data.frame]} a validation report derived from one of the +\code{validate} functions.} +} +\value{ +NULL, invisibly. This is used for it's side-effect of formatting and +issuing messages via \code{\link[=issue_warning]{issue_warning()}}. +} +\description{ +Collapse a variable number of validation reports into a single message that +can be formatted for the CLI or GitHub. +} +\details{ +One of the key features of {pegboard} is the ability to parse and +validate markdown elements. These functions provide a standard way of +creating the reports that are for the user based on whether or not they are +on the CLI or on GitHub. The prerequisites of these functions are the input +data frame (generated from the actual validation function) and an internal +set of known templating vectors that contain templates for each test to show +the actual error along with general information that can help correct the +error (see below). +\subsection{Input Data Frame}{ + +The validations are initially reported in a data frame that has the +following properties: +\itemize{ +\item one row per element +\item columns that indicate the parsed attributes of the element, source +file, source position, and the actual element XML node object. +\item boolean columns that indicate the tests for each element, used with +\code{\link[=collect_labels]{collect_labels()}} to add a "labels" column to the data. +} +} + +\subsection{Templating vectors}{ + +These vectors come in two forms \verb{[thing]_tests} and \verb{[thing]_info} (e.g. +for \code{\link[=validate_links]{validate_links()}}, we have \code{link_tests} and \code{link_info}). These are +named vectors that match the boolean columns of the data frame produced +by the validation function. The \verb{[thing]_tests} vector contains templates +that describes the error and shows the text that caused the error. The +\verb{[thing]_info} contains general information about how to address that +particular error. For example, one common link error is that a link is not +descriptive (e.g. the link text says "click here"). The column in the \code{VAL} +data frame that contains the result of this test is called "descriptive", so +if we look at the values from the link info and tests vectors: + +\if{html}{\out{
}}\preformatted{link_info["descriptive"] +#> descriptive +#> "Avoid uninformative link phrases " +link_tests["descriptive"] +#> descriptive +#> "[uninformative link text]: [\{text\}](\{orig\})" +}\if{html}{\out{
}} + +If the \verb{throw_*_warnings()} functions detect any errors, they will use the +info and tests vectors to construct a composite message. +} + +\subsection{Process}{ + +The \verb{throw_*_warnings()} functions all do the same basic procedure (and +indeed could be consolidated into a single function in the future) +\enumerate{ +\item pass data to \code{\link[=collect_labels]{collect_labels()}}, which will parse the \verb{[thing]_tests} +templating vector and label each failing element in \code{VAL} with the +appropriate failure message +\item gather the source information for each failure +\item pass failures with the \verb{[thing]_info} elements that matched the unique +failures to \code{\link[=issue_warning]{issue_warning()}} +} +} +} +\seealso{ +\code{\link[=validate_links]{validate_links()}}, \code{\link[=validate_divs]{validate_divs()}}, and \code{\link[=validate_headings]{validate_headings()}} for +input sources for these functions. +} diff --git a/tests/testthat/_snaps/Lesson.md b/tests/testthat/_snaps/Lesson.md index 8591d414..708f0953 100644 --- a/tests/testthat/_snaps/Lesson.md +++ b/tests/testthat/_snaps/Lesson.md @@ -269,7 +269,7 @@ Code vlink <- frg$validate_links() Message - ! There were errors in 4/14 links + ! There were errors in 4/14 images ( ) Some linked internal files do not exist ( ) Images need alt-text @@ -350,7 +350,7 @@ Code vlink <- frg$validate_links() Message - ! There were errors in 4/14 links + ! There were errors in 4/14 images ( ) Some linked internal files do not exist ( ) Images need alt-text @@ -431,7 +431,7 @@ Code vlink <- frg$validate_links() Message - ! There were errors in 4/14 links + ! There were errors in 4/14 images ◌ Some linked internal files do not exist ◌ Images need alt-text @@ -512,7 +512,7 @@ Code vlink <- frg$validate_links() Message - ! There were errors in 4/14 links + ! There were errors in 4/14 images ◌ Some linked internal files do not exist ◌ Images need alt-text diff --git a/tests/testthat/_snaps/validation.md b/tests/testthat/_snaps/validation.md index ca9a2b7e..e29b9af6 100644 --- a/tests/testthat/_snaps/validation.md +++ b/tests/testthat/_snaps/validation.md @@ -67,7 +67,7 @@ Code cats$validate_links() Message - ! There were errors in 2/10 links + ! There were errors in 2/10 images - Images need alt-text @@ -79,7 +79,7 @@ Code loop$validate_links() Message - ! There were errors in 4/13 links + ! There were errors in 4/13 images - Some linked internal files do not exist - Images need alt-text @@ -265,7 +265,7 @@ Code cats$validate_links() Message - ! There were errors in 2/10 links + ! There were errors in 2/10 images ( ) Images need alt-text image-test.md:12 [image missing alt-text]: https://placekitten.com/g/102/102 @@ -322,7 +322,7 @@ Code cats$validate_links() Message - ! There were errors in 2/10 links + ! There were errors in 2/10 images ( ) Images need alt-text image-test.md:12 [image missing alt-text]: https://placekitten.com/g/102/102 @@ -379,7 +379,7 @@ Code cats$validate_links() Message - ! There were errors in 2/10 links + ! There were errors in 2/10 images ◌ Images need alt-text image-test.md:12 [image missing alt-text]: https://placekitten.com/g/102/102 @@ -436,7 +436,7 @@ Code cats$validate_links() Message - ! There were errors in 2/10 links + ! There were errors in 2/10 images ◌ Images need alt-text image-test.md:12 [image missing alt-text]: https://placekitten.com/g/102/102 From f8046ea44fc2031648884fec1207806f500d79b4 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 16 Nov 2023 11:24:25 -0800 Subject: [PATCH 3/6] update test --- tests/testthat/test-Lesson.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-Lesson.R b/tests/testthat/test-Lesson.R index 08857f3e..77a890bc 100644 --- a/tests/testthat/test-Lesson.R +++ b/tests/testthat/test-Lesson.R @@ -229,7 +229,7 @@ if (requireNamespace("cli")) { test_that("Lessons can be _quietly_ validated", { suppressMessages({ expect_message(vhead <- frg$validate_headings(verbose = FALSE), "There were errors in 13/37 headings") - expect_message(vlink <- frg$validate_links(), "There were errors in 4/14 links") + expect_message(vlink <- frg$validate_links(), "There were errors in 4/14 images") }) }) From ae7c834c842986792592583f64d0288ff64cc6d5 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 16 Nov 2023 11:57:54 -0800 Subject: [PATCH 4/6] fix documentation --- NAMESPACE | 1 - R/utils-validation.R | 40 ++++++++++++++++++++++++++++++--- man/collect_labels.Rd | 52 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 man/collect_labels.Rd diff --git a/NAMESPACE b/NAMESPACE index 68541af4..a9860836 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -16,7 +16,6 @@ export(lesson_fragment) export(link_tests) export(liquid_to_commonmark) export(make_link_table) -export(throw_heading_warnings) importFrom(R6,R6Class) importFrom(glue,glue) importFrom(purrr,map) diff --git a/R/utils-validation.R b/R/utils-validation.R index 985fd2c2..dca26529 100644 --- a/R/utils-validation.R +++ b/R/utils-validation.R @@ -7,7 +7,6 @@ #' `validate` functions. #' @return NULL, invisibly. This is used for it's side-effect of formatting and #' issuing messages via [issue_warning()]. -#' @export #' @details One of the key features of {pegboard} is the ability to parse and #' validate markdown elements. These functions provide a standard way of #' creating the reports that are for the user based on whether or not they are @@ -132,10 +131,45 @@ throw_link_warnings <- function(VAL) { reports = reports) } +#' Collect and append validation messages +#' +#' Given a data frame containing the results of validation tests, this will +#' append a column of labels that describes each failure. +#' #' @param VAL a data frame containing the results of tests #' @param cli indicator to use the cli package to format warnings -#' @param msg (collect_labels) a named vector of messages to provide for each test -#' @noRd +#' @param msg a named vector of template messages to provide for each test +#' formatted for the \pkg{glue} package. +#' +#' @seealso [throw_link_warnings()] for details on how this is implemented. +#' @examples +#' # As an example, consider a data frame where you have observations in rows +#' # and the results of individual tests in columns: +#' set.seed(2023-11-16) +#' dat <- data.frame( +#' name = letters[1:10], +#' rank = sample(1:3, 10, replace = TRUE), +#' A = sample(c(TRUE, FALSE), 10, replace = TRUE, prob = c(7, 3)), +#' B = sample(c(TRUE, FALSE), 10, replace = TRUE, prob = c(7, 3)), +#' C = sample(c(TRUE, FALSE), 10, replace = TRUE, prob = c(7, 3)) +#' ) +#' dat +#' # you can see what the results of the tests were, but it would be a good +#' # idea to have a lookup table describing what these results mean +#' dat_tests <- c( +#' A = "[missing widget]: {name}", +#' B = "[incorrect rank]: {rank}", +#' C = "[something else]" +#' ) +#' # collect_labels will create the output you need: +#' res <- collect_labels(dat, msg = dat_tests) +#' res +#' writeLines(res$labels) +#' if (requireNamespace("cli", quietly = TRUE)) { +#' # you can also specify cli to TRUE to format with CLI +#' res <- collect_labels(dat, cli = TRUE, msg = dat_tests) +#' writeLines(res$labels) +#' } collect_labels <- function(VAL, cli = FALSE, msg = heading_tests) { labels <- character(nrow(VAL)) for (test in names(msg)) { diff --git a/man/collect_labels.Rd b/man/collect_labels.Rd new file mode 100644 index 00000000..113cd54e --- /dev/null +++ b/man/collect_labels.Rd @@ -0,0 +1,52 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/utils-validation.R +\name{collect_labels} +\alias{collect_labels} +\title{Collect and append validation messages} +\usage{ +collect_labels(VAL, cli = FALSE, msg = heading_tests) +} +\arguments{ +\item{VAL}{a data frame containing the results of tests} + +\item{cli}{indicator to use the cli package to format warnings} + +\item{msg}{a named vector of template messages to provide for each test +formatted for the \pkg{glue} package.} +} +\description{ +Given a data frame containing the results of validation tests, this will +append a column of labels that describes each failure. +} +\examples{ +# As an example, consider a data frame where you have observations in rows +# and the results of individual tests in columns: +set.seed(2023-11-16) +dat <- data.frame( + name = letters[1:10], + rank = sample(1:3, 10, replace = TRUE), + A = sample(c(TRUE, FALSE), 10, replace = TRUE, prob = c(7, 3)), + B = sample(c(TRUE, FALSE), 10, replace = TRUE, prob = c(7, 3)), + C = sample(c(TRUE, FALSE), 10, replace = TRUE, prob = c(7, 3)) +) +dat +# you can see what the results of the tests were, but it would be a good +# idea to have a lookup table describing what these results mean +dat_tests <- c( + A = "[missing widget]: {name}", + B = "[incorrect rank]: {rank}", + C = "[something else]" +) +# collect_labels will create the output you need: +res <- collect_labels(dat, msg = dat_tests) +res +writeLines(res$labels) +if (requireNamespace("cli", quietly = TRUE)) { + # you can also specify cli to TRUE to format with CLI + res <- collect_labels(dat, cli = TRUE, msg = dat_tests) + writeLines(res$labels) +} +} +\seealso{ +\code{\link[=throw_link_warnings]{throw_link_warnings()}} for details on how this is implemented. +} From c2b8bb5136a4f3cc4708170ff423d6138eb8511d Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 16 Nov 2023 11:59:57 -0800 Subject: [PATCH 5/6] fix broken examples --- R/utils-validation.R | 5 +++-- man/collect_labels.Rd | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/R/utils-validation.R b/R/utils-validation.R index dca26529..8fcc6fe5 100644 --- a/R/utils-validation.R +++ b/R/utils-validation.R @@ -162,12 +162,13 @@ throw_link_warnings <- function(VAL) { #' C = "[something else]" #' ) #' # collect_labels will create the output you need: -#' res <- collect_labels(dat, msg = dat_tests) +#' pb <- asNamespace("pegboard") +#' res <- pb$collect_labels(dat, msg = dat_tests) #' res #' writeLines(res$labels) #' if (requireNamespace("cli", quietly = TRUE)) { #' # you can also specify cli to TRUE to format with CLI -#' res <- collect_labels(dat, cli = TRUE, msg = dat_tests) +#' res <- pb$collect_labels(dat, cli = TRUE, msg = dat_tests) #' writeLines(res$labels) #' } collect_labels <- function(VAL, cli = FALSE, msg = heading_tests) { diff --git a/man/collect_labels.Rd b/man/collect_labels.Rd index 113cd54e..3b623299 100644 --- a/man/collect_labels.Rd +++ b/man/collect_labels.Rd @@ -38,12 +38,13 @@ dat_tests <- c( C = "[something else]" ) # collect_labels will create the output you need: -res <- collect_labels(dat, msg = dat_tests) +pb <- asNamespace("pegboard") +res <- pb$collect_labels(dat, msg = dat_tests) res writeLines(res$labels) if (requireNamespace("cli", quietly = TRUE)) { # you can also specify cli to TRUE to format with CLI - res <- collect_labels(dat, cli = TRUE, msg = dat_tests) + res <- pb$collect_labels(dat, cli = TRUE, msg = dat_tests) writeLines(res$labels) } } From c6e3967c65840d913b5b936de437190e3b9ecfc3 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 16 Nov 2023 12:03:24 -0800 Subject: [PATCH 6/6] update pkgdown --- _pkgdown.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/_pkgdown.yml b/_pkgdown.yml index a4a73e0c..bef2660c 100644 --- a/_pkgdown.yml +++ b/_pkgdown.yml @@ -30,6 +30,8 @@ reference: - validate_headings - validate_links - validate_divs + - throw_heading_warnings + - collect_labels - issue_warning - show_heading_tree - title: "Helper functions"