From 8513981d416ba3698a0a54645e4fb7373be106e7 Mon Sep 17 00:00:00 2001 From: olivroy Date: Sat, 18 May 2024 14:17:32 -0400 Subject: [PATCH 01/11] Remove remotes --- DESCRIPTION | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 305333a7d..72aa52cf9 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -34,7 +34,7 @@ Imports: purrr (>= 1.0.0), ragg, rlang (>= 1.1.0), - rmarkdown (>= 2.26.2), + rmarkdown (>= 2.27), tibble, whisker, withr (>= 2.4.3), @@ -72,5 +72,3 @@ Encoding: UTF-8 Roxygen: list(markdown = TRUE) RoxygenNote: 7.3.1 SystemRequirements: pandoc -Remotes: - rstudio/rmarkdown From e409a550a4f04daf0fc8a7862429d1255f535819 Mon Sep 17 00:00:00 2001 From: olivroy Date: Sat, 18 May 2024 15:35:54 -0400 Subject: [PATCH 02/11] Proof of concept to rethrow yaml parsing error in _pkgdown.yml --- R/package.R | 90 ++++++++++++++++++++- tests/testthat/_snaps/package.md | 13 +++ tests/testthat/assets/bad-yaml/_pkgdown.yml | 16 ++++ tests/testthat/test-package.R | 11 +++ 4 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 tests/testthat/assets/bad-yaml/_pkgdown.yml diff --git a/R/package.R b/R/package.R index 73214a06c..84de4a913 100644 --- a/R/package.R +++ b/R/package.R @@ -178,13 +178,97 @@ pkgdown_config_path <- function(path) { ) } -read_meta <- function(path) { - path <- pkgdown_config_path(path) +read_meta <- function(path, check_path = TRUE) { + if (check_path) { + # check_path = FALSE can be used to supply a direct path to + # read_meta instead of a pkgdown object. + path <- pkgdown_config_path(path) + } + # If parsing fails, it will say Error in read_meta() + call <- current_env() if (is.null(path)) { yaml <- list() } else { - yaml <- yaml::yaml.load_file(path) %||% list() + yaml <- withCallingHandlers( + yaml::yaml.load_file(path), + error = function(e) { + yaml_error_msg <- conditionMessage(e) + # Create a clickable path, in case we end up + # just rethrowing the original message + yaml_error_msg <- gsub( + "\\(([^\\)]+)\\)", + "({.path \\1})", + yaml_error_msg + ) + + # trying to extract the parsing error type? + # like block mapping, block collection? + what <- regmatches( + yaml_error_msg, + m = regexpr("block\\s[[:alpha:]]+", yaml_error_msg) + ) + # search for all occurences of digits (1 to 4 digits long) + # I would not see a _pkgdown.yml file containing >9999 lines. + error_locations <- regmatches( + yaml_error_msg, + m = gregexpr("\\d{1,4}", yaml_error_msg) + ) + error_locations <- unlist(error_locations, use.names = FALSE) + if (length(error_locations) != 4 || !is_string(what)) { + # can't parse the error message properly, + # just rethrow it cli style + cli::cli_abort(yaml_error_msg, call = call) + } + # yaml throws its message like this: + # ! (./pkgdown/_pkgdown.yml) Parser error: while parsing + # a block mapping at line 1, column 1 + # did not find expected key at line 9, column 3 + block_line <- error_locations[1] + block_col <- error_locations[2] + error_line <- error_locations[3] + error_col <- error_locations[4] + + # try to identify the faulty block / field name to add a hint + block_name <- withCallingHandlers( + readLines(path, warn = FALSE, n = block_line, encoding = "UTF-8"), + error = function(e) NULL + ) + if (!is.null(block_name)) { + block_name <- block_name[length(block_name)] + block_name <- gsub(":.*$", ":", block_name) + } + + block_pos <- cli::style_hyperlink( + paste0(block_line), + url = paste0("file://", path), + params = list( + line = as.integer(block_line), + col = as.integer(block_col) + ) + ) + error_pos <- cli::style_hyperlink( + paste0(error_line), + url = paste0("file://", path), + params = list( + line = as.integer(error_line), + col = as.integer(error_col) + ) + ) + cli::cli_abort( + c( + "!" = "The {.field {block_name}} field failed to parse.", + "i" = "Error occured between lines {block_pos} and {error_pos}.", + "i" = "Edit {.path {path}} to fix the problem.", + # repeat the yaml error + "i" = "Did not find the expected keys at line {error_pos} while parsing a {what}." + ), + call = call, + parent = e + ) + } + ) + yaml <- yaml %||% list() } yaml diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index b29f034fd..bcd71eb60 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -20,3 +20,16 @@ ! template.bootstrap must be 3 or 5, not 1. i Edit _pkgdown.yml to fix the problem. +# read_meta() errors gracefully if _pkgdown.yml failed to parse + + Code + read_meta(file, check_path = FALSE) + Condition + Error in `read_meta()`: + ! The url: field failed to parse. + i Error occured between lines 1 and 9. + i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the problem. + i Did not find the expected keys at line 9 while parsing a block mapping. + Caused by error in `yaml.load()`: + ! (assets/bad-yaml/_pkgdown.yml) Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 + diff --git a/tests/testthat/assets/bad-yaml/_pkgdown.yml b/tests/testthat/assets/bad-yaml/_pkgdown.yml new file mode 100644 index 000000000..159087c1d --- /dev/null +++ b/tests/testthat/assets/bad-yaml/_pkgdown.yml @@ -0,0 +1,16 @@ +url: https://pkgdown.r-lib.org + +home: + title: Build websites for R packages + +authors: + Jay Hesselberth: + href: https://hesselberthlab.org + Maëlle Salmon: + href: https://masalmon.eu + Hadley Wickham: + href: http://hadley.nz + Posit Software, PBC: + href: https://www.posit.co + html: >- + Posit diff --git a/tests/testthat/test-package.R b/tests/testthat/test-package.R index f7fa88fec..2723feded 100644 --- a/tests/testthat/test-package.R +++ b/tests/testthat/test-package.R @@ -82,3 +82,14 @@ test_that("titles don't get autolinked code", { rd <- rd_text("\\title{\\code{foo()}}", fragment = FALSE) expect_equal(extract_title(rd), "foo()") }) + +test_that("read_meta() errors gracefully if _pkgdown.yml failed to parse", { + file <- test_path("assets", "bad-yaml", "_pkgdown.yml") + expect_snapshot( + error = TRUE, + read_meta( + file, + check_path = FALSE + ) + ) +}) From ca58a24d5d29d6e8dab5fc4b44a48b064e42393a Mon Sep 17 00:00:00 2001 From: olivroy Date: Sat, 18 May 2024 15:48:58 -0400 Subject: [PATCH 03/11] lint + add comment --- R/package.R | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/R/package.R b/R/package.R index 84de4a913..13f528132 100644 --- a/R/package.R +++ b/R/package.R @@ -230,8 +230,11 @@ read_meta <- function(path, check_path = TRUE) { error_col <- error_locations[4] # try to identify the faulty block / field name to add a hint + # i.e. the block at where the parser gives his first error. + # but it may be more accurate to try to detect a field / block + # looking backward from the second position mentioned in the error. block_name <- withCallingHandlers( - readLines(path, warn = FALSE, n = block_line, encoding = "UTF-8"), + read_lines(path, n = block_line), error = function(e) NULL ) if (!is.null(block_name)) { From 90222bbd651f63a9b1b9ca8f7bd39e8f3f25a326 Mon Sep 17 00:00:00 2001 From: olivroy Date: Sat, 18 May 2024 16:22:50 -0400 Subject: [PATCH 04/11] Simplify output and use `call = caller_env()` after all (so that the word pkgdown will show in real life situation. --- R/package.R | 100 ++++++------------------------- tests/testthat/_snaps/package.md | 9 +-- tests/testthat/test-package.R | 4 +- 3 files changed, 23 insertions(+), 90 deletions(-) diff --git a/R/package.R b/R/package.R index 13f528132..47f19f3f2 100644 --- a/R/package.R +++ b/R/package.R @@ -178,102 +178,38 @@ pkgdown_config_path <- function(path) { ) } -read_meta <- function(path, check_path = TRUE) { +read_meta <- function(path, check_path = TRUE, call = caller_env()) { if (check_path) { # check_path = FALSE can be used to supply a direct path to # read_meta instead of a pkgdown object. path <- pkgdown_config_path(path) } - # If parsing fails, it will say Error in read_meta() - call <- current_env() if (is.null(path)) { yaml <- list() } else { yaml <- withCallingHandlers( - yaml::yaml.load_file(path), + yaml::yaml.load_file(path) %||% list(), error = function(e) { - yaml_error_msg <- conditionMessage(e) - # Create a clickable path, in case we end up - # just rethrowing the original message - yaml_error_msg <- gsub( - "\\(([^\\)]+)\\)", - "({.path \\1})", - yaml_error_msg - ) - - # trying to extract the parsing error type? - # like block mapping, block collection? - what <- regmatches( - yaml_error_msg, - m = regexpr("block\\s[[:alpha:]]+", yaml_error_msg) - ) - # search for all occurences of digits (1 to 4 digits long) - # I would not see a _pkgdown.yml file containing >9999 lines. - error_locations <- regmatches( - yaml_error_msg, - m = gregexpr("\\d{1,4}", yaml_error_msg) - ) - error_locations <- unlist(error_locations, use.names = FALSE) - if (length(error_locations) != 4 || !is_string(what)) { - # can't parse the error message properly, - # just rethrow it cli style - cli::cli_abort(yaml_error_msg, call = call) - } - # yaml throws its message like this: - # ! (./pkgdown/_pkgdown.yml) Parser error: while parsing - # a block mapping at line 1, column 1 - # did not find expected key at line 9, column 3 - block_line <- error_locations[1] - block_col <- error_locations[2] - error_line <- error_locations[3] - error_col <- error_locations[4] - - # try to identify the faulty block / field name to add a hint - # i.e. the block at where the parser gives his first error. - # but it may be more accurate to try to detect a field / block - # looking backward from the second position mentioned in the error. - block_name <- withCallingHandlers( - read_lines(path, n = block_line), - error = function(e) NULL - ) - if (!is.null(block_name)) { - block_name <- block_name[length(block_name)] - block_name <- gsub(":.*$", ":", block_name) - } - - block_pos <- cli::style_hyperlink( - paste0(block_line), - url = paste0("file://", path), - params = list( - line = as.integer(block_line), - col = as.integer(block_col) - ) - ) - error_pos <- cli::style_hyperlink( - paste0(error_line), - url = paste0("file://", path), - params = list( - line = as.integer(error_line), - col = as.integer(error_col) - ) - ) - cli::cli_abort( - c( - "!" = "The {.field {block_name}} field failed to parse.", - "i" = "Error occured between lines {block_pos} and {error_pos}.", - "i" = "Edit {.path {path}} to fix the problem.", - # repeat the yaml error - "i" = "Did not find the expected keys at line {error_pos} while parsing a {what}." + # Tweak the original message to put the location of the error at the end + # based on yaml 2.3.8 error message structure + # (<>) Parser error: <> + yaml_err <- conditionMessage(e) + # extract parsing error from original error (i.e. remove the path) + parsing_error <- sub("[^\\)]+\\)", "", yaml_err) + # Extract path from original error + path_yaml <- regmatches(yaml_err, m = regexpr("\\(([^\\)]+)\\)", yaml_err)) + path_yaml <- gsub("\\(([^\\)]+)\\)", "\\1", path_yaml) + # Rethrow cli-styled error! + cli::cli_abort(c( + "x" = "Could not parse the config file.", + "!" = parsing_error, + "i" = "Edit {.path {path_yaml}} to fix the problem." ), - call = call, - parent = e + call = call ) - } - ) - yaml <- yaml %||% list() + }) } - yaml } diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index bcd71eb60..00cf48f8e 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -25,11 +25,8 @@ Code read_meta(file, check_path = FALSE) Condition - Error in `read_meta()`: - ! The url: field failed to parse. - i Error occured between lines 1 and 9. + Error: + x Could not parse the config file. + ! Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the problem. - i Did not find the expected keys at line 9 while parsing a block mapping. - Caused by error in `yaml.load()`: - ! (assets/bad-yaml/_pkgdown.yml) Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 diff --git a/tests/testthat/test-package.R b/tests/testthat/test-package.R index 2723feded..ca56ca769 100644 --- a/tests/testthat/test-package.R +++ b/tests/testthat/test-package.R @@ -56,10 +56,10 @@ test_that("package_vignettes() sorts articles alphabetically by file name", { test_that("override works correctly for as_pkgdown", { pkgdown <- as_pkgdown(test_path("assets/articles-images")) - + expected_list <- list(dev = "jpeg", fig.ext = "jpg", fig.width = 3, fig.asp = 1) expect_equal(pkgdown$meta$figures, expected_list) - + modified_pkgdown <- as_pkgdown(pkgdown, override = list(figures = list(dev = "png"))) expect_equal(modified_pkgdown$meta$figures$dev, "png") }) From 04067b99a1a9e58332a1c87329691a4f178f6ddc Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 19 May 2024 09:57:39 -0400 Subject: [PATCH 05/11] Remove extra space --- R/package.R | 2 +- tests/testthat/_snaps/package.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/package.R b/R/package.R index 47f19f3f2..6747f92f7 100644 --- a/R/package.R +++ b/R/package.R @@ -196,7 +196,7 @@ read_meta <- function(path, check_path = TRUE, call = caller_env()) { # (<>) Parser error: <> yaml_err <- conditionMessage(e) # extract parsing error from original error (i.e. remove the path) - parsing_error <- sub("[^\\)]+\\)", "", yaml_err) + parsing_error <- sub("[^\\)]+\\)\\s", "", yaml_err) # Extract path from original error path_yaml <- regmatches(yaml_err, m = regexpr("\\(([^\\)]+)\\)", yaml_err)) path_yaml <- gsub("\\(([^\\)]+)\\)", "\\1", path_yaml) diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index 00cf48f8e..01fdc3913 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -27,6 +27,6 @@ Condition Error: x Could not parse the config file. - ! Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 + ! Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the problem. From 4661e5b96b23772b87f02fd0ab7dfb5f49586f3b Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 19 May 2024 10:20:13 -0400 Subject: [PATCH 06/11] No longer need the `check_path` hack as this approach will not require testing different `_pkgdown.yml` error messages. --- R/package.R | 9 ++------- tests/testthat/_snaps/package.md | 4 ++-- tests/testthat/assets/bad-yaml/DESCRIPTION | 13 +++++++++++++ tests/testthat/test-package.R | 6 +----- 4 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 tests/testthat/assets/bad-yaml/DESCRIPTION diff --git a/R/package.R b/R/package.R index 6747f92f7..821144c8c 100644 --- a/R/package.R +++ b/R/package.R @@ -178,13 +178,8 @@ pkgdown_config_path <- function(path) { ) } -read_meta <- function(path, check_path = TRUE, call = caller_env()) { - if (check_path) { - # check_path = FALSE can be used to supply a direct path to - # read_meta instead of a pkgdown object. - path <- pkgdown_config_path(path) - } - +read_meta <- function(path, call = caller_env()) { + path <- pkgdown_config_path(path) if (is.null(path)) { yaml <- list() } else { diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index 01fdc3913..7482d9d7a 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -23,9 +23,9 @@ # read_meta() errors gracefully if _pkgdown.yml failed to parse Code - read_meta(file, check_path = FALSE) + as_pkgdown(test_path("assets/bad-yaml")) Condition - Error: + Error in `as_pkgdown()`: x Could not parse the config file. ! Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the problem. diff --git a/tests/testthat/assets/bad-yaml/DESCRIPTION b/tests/testthat/assets/bad-yaml/DESCRIPTION new file mode 100644 index 000000000..3da3a37bf --- /dev/null +++ b/tests/testthat/assets/bad-yaml/DESCRIPTION @@ -0,0 +1,13 @@ +Package: kittens +Version: 1.0.0 +Title: A test package +Description: A longer statement about the package. +Authors@R: c( + person("Hadley", "Wickham", , "hadley@rstudio.com", role = c("aut", "cre")), + person("RStudio", role = c("cph", "fnd")) + ) +RoxygenNote: 6.0.1 +Suggests: + knitr, + rmarkdown +VignetteBuilder: knitr diff --git a/tests/testthat/test-package.R b/tests/testthat/test-package.R index ca56ca769..ffd061f51 100644 --- a/tests/testthat/test-package.R +++ b/tests/testthat/test-package.R @@ -84,12 +84,8 @@ test_that("titles don't get autolinked code", { }) test_that("read_meta() errors gracefully if _pkgdown.yml failed to parse", { - file <- test_path("assets", "bad-yaml", "_pkgdown.yml") expect_snapshot( error = TRUE, - read_meta( - file, - check_path = FALSE - ) + as_pkgdown(test_path("assets/bad-yaml")) ) }) From ae00cad5c8e1aa366a699b8620a0b52c541bd599 Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 19 May 2024 10:25:50 -0400 Subject: [PATCH 07/11] Simplify test files and revert ws change --- R/package.R | 4 +++- tests/testthat/_snaps/package.md | 2 +- tests/testthat/assets/bad-yaml/DESCRIPTION | 12 +----------- tests/testthat/assets/bad-yaml/_pkgdown.yml | 10 ---------- 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/R/package.R b/R/package.R index 821144c8c..17c67bd40 100644 --- a/R/package.R +++ b/R/package.R @@ -180,6 +180,7 @@ pkgdown_config_path <- function(path) { read_meta <- function(path, call = caller_env()) { path <- pkgdown_config_path(path) + if (is.null(path)) { yaml <- list() } else { @@ -191,7 +192,7 @@ read_meta <- function(path, call = caller_env()) { # (<>) Parser error: <> yaml_err <- conditionMessage(e) # extract parsing error from original error (i.e. remove the path) - parsing_error <- sub("[^\\)]+\\)\\s", "", yaml_err) + parsing_error <- sub("[^\\)]+\\)\\s?", "", yaml_err) # Extract path from original error path_yaml <- regmatches(yaml_err, m = regexpr("\\(([^\\)]+)\\)", yaml_err)) path_yaml <- gsub("\\(([^\\)]+)\\)", "\\1", path_yaml) @@ -205,6 +206,7 @@ read_meta <- function(path, call = caller_env()) { ) }) } + yaml } diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index 7482d9d7a..4f782273f 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -27,6 +27,6 @@ Condition Error in `as_pkgdown()`: x Could not parse the config file. - ! Parser error: while parsing a block mapping at line 1, column 1 did not find expected key at line 9, column 3 + ! Scanner error: mapping values are not allowed in this context at line 2, column 8 i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the problem. diff --git a/tests/testthat/assets/bad-yaml/DESCRIPTION b/tests/testthat/assets/bad-yaml/DESCRIPTION index 3da3a37bf..2329d143b 100644 --- a/tests/testthat/assets/bad-yaml/DESCRIPTION +++ b/tests/testthat/assets/bad-yaml/DESCRIPTION @@ -1,13 +1,3 @@ Package: kittens -Version: 1.0.0 -Title: A test package -Description: A longer statement about the package. -Authors@R: c( - person("Hadley", "Wickham", , "hadley@rstudio.com", role = c("aut", "cre")), - person("RStudio", role = c("cph", "fnd")) - ) -RoxygenNote: 6.0.1 -Suggests: - knitr, - rmarkdown +Authors@R: person("Hadley", "Wickham", , "hadley@rstudio.com", role = c("aut", "cre")), VignetteBuilder: knitr diff --git a/tests/testthat/assets/bad-yaml/_pkgdown.yml b/tests/testthat/assets/bad-yaml/_pkgdown.yml index 159087c1d..01904824d 100644 --- a/tests/testthat/assets/bad-yaml/_pkgdown.yml +++ b/tests/testthat/assets/bad-yaml/_pkgdown.yml @@ -1,16 +1,6 @@ url: https://pkgdown.r-lib.org - -home: title: Build websites for R packages authors: Jay Hesselberth: href: https://hesselberthlab.org - Maëlle Salmon: - href: https://masalmon.eu - Hadley Wickham: - href: http://hadley.nz - Posit Software, PBC: - href: https://www.posit.co - html: >- - Posit From 1ef569de38cc934416621c88632eb27e2b240f92 Mon Sep 17 00:00:00 2001 From: olivroy Date: Tue, 21 May 2024 07:36:15 -0400 Subject: [PATCH 08/11] use helper function + take advantage of `error.label = NULL` to suppress path from original error message. --- R/package.R | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/R/package.R b/R/package.R index 17c67bd40..8c550cd89 100644 --- a/R/package.R +++ b/R/package.R @@ -184,32 +184,33 @@ read_meta <- function(path, call = caller_env()) { if (is.null(path)) { yaml <- list() } else { - yaml <- withCallingHandlers( - yaml::yaml.load_file(path) %||% list(), - error = function(e) { - # Tweak the original message to put the location of the error at the end - # based on yaml 2.3.8 error message structure - # (<>) Parser error: <> - yaml_err <- conditionMessage(e) - # extract parsing error from original error (i.e. remove the path) - parsing_error <- sub("[^\\)]+\\)\\s?", "", yaml_err) - # Extract path from original error - path_yaml <- regmatches(yaml_err, m = regexpr("\\(([^\\)]+)\\)", yaml_err)) - path_yaml <- gsub("\\(([^\\)]+)\\)", "\\1", path_yaml) - # Rethrow cli-styled error! - cli::cli_abort(c( - "x" = "Could not parse the config file.", - "!" = parsing_error, - "i" = "Edit {.path {path_yaml}} to fix the problem." - ), - call = call - ) - }) + yaml <- cli_yaml_load(path, call = call) %||% list() } - yaml } +# Wrapper around yaml::yaml.load_file() +cli_yaml_load <- function(path, call = caller_env()) { + # Tweak the original message to put the location of the error at the end + withCallingHandlers( + yaml::yaml.load_file( + path, + error.label = NULL # suppress path from message. + ), + error = function(e) { + yaml_err <- conditionMessage(e) + # Rethrow cli-styled error! + cli::cli_abort(c( + "x" = "Could not parse the config file.", + "!" = yaml_err, + "i" = "Edit {.path {path}} to fix the problem." + ), + call = call + ) + } + ) +} + # Topics ------------------------------------------------------------------ package_topics <- function(path = ".", package = "pkgdown") { From fa934ac325918e8299a3a76987b65106fbc2bceb Mon Sep 17 00:00:00 2001 From: olivroy Date: Tue, 21 May 2024 09:27:56 -0400 Subject: [PATCH 09/11] use parent = e for simplicity --- R/package.R | 35 +++++++++++--------------------- tests/testthat/_snaps/package.md | 3 ++- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/R/package.R b/R/package.R index f06242b4c..48017cb5d 100644 --- a/R/package.R +++ b/R/package.R @@ -188,33 +188,22 @@ read_meta <- function(path, call = caller_env()) { if (is.null(path)) { yaml <- list() } else { - yaml <- cli_yaml_load(path, call = call) %||% list() + yaml <- withCallingHandlers( + yaml::yaml.load_file(path, error.label = NULL) %||% list(), + error = function(e) { + cli::cli_abort(c( + "x" = "Could not parse the config file.", + "i" = "Edit {.path {path}} to fix the probkem." + ), + call = call, + parent = e + ) + } + ) } yaml } -# Wrapper around yaml::yaml.load_file() -cli_yaml_load <- function(path, call = caller_env()) { - # Tweak the original message to put the location of the error at the end - withCallingHandlers( - yaml::yaml.load_file( - path, - error.label = NULL # suppress path from message. - ), - error = function(e) { - yaml_err <- conditionMessage(e) - # Rethrow cli-styled error! - cli::cli_abort(c( - "x" = "Could not parse the config file.", - "!" = yaml_err, - "i" = "Edit {.path {path}} to fix the problem." - ), - call = call - ) - } - ) -} - # Topics ------------------------------------------------------------------ package_topics <- function(path = ".", package = "pkgdown") { diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index 4f782273f..8fa78f196 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -27,6 +27,7 @@ Condition Error in `as_pkgdown()`: x Could not parse the config file. + i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the probkem. + Caused by error in `yaml.load()`: ! Scanner error: mapping values are not allowed in this context at line 2, column 8 - i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the problem. From d79316f9efaa7ee465e8c192829aaa8fc8bb689f Mon Sep 17 00:00:00 2001 From: olivroy Date: Tue, 21 May 2024 09:35:23 -0400 Subject: [PATCH 10/11] Accept comment + snap --- R/package.R | 6 ++---- tests/testthat/_snaps/package.md | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/R/package.R b/R/package.R index 48017cb5d..8fab37bda 100644 --- a/R/package.R +++ b/R/package.R @@ -191,10 +191,8 @@ read_meta <- function(path, call = caller_env()) { yaml <- withCallingHandlers( yaml::yaml.load_file(path, error.label = NULL) %||% list(), error = function(e) { - cli::cli_abort(c( - "x" = "Could not parse the config file.", - "i" = "Edit {.path {path}} to fix the probkem." - ), + cli::cli_abort( + "Could not parse config file at {.path {path}}.", call = call, parent = e ) diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index 8fa78f196..3f6976305 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -26,8 +26,7 @@ as_pkgdown(test_path("assets/bad-yaml")) Condition Error in `as_pkgdown()`: - x Could not parse the config file. - i Edit 'assets/bad-yaml/_pkgdown.yml' to fix the probkem. + ! Could not parse config file at 'assets/bad-yaml/_pkgdown.yml'. Caused by error in `yaml.load()`: ! Scanner error: mapping values are not allowed in this context at line 2, column 8 From dfed20deafb187405a9591ea66d03adc0269ca14 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 21 May 2024 14:49:23 -0500 Subject: [PATCH 11/11] Use inline test --- tests/testthat/_snaps/package.md | 4 ++-- tests/testthat/assets/bad-yaml/DESCRIPTION | 3 --- tests/testthat/assets/bad-yaml/_pkgdown.yml | 6 ------ tests/testthat/test-package.R | 8 +++++++- 4 files changed, 9 insertions(+), 12 deletions(-) delete mode 100644 tests/testthat/assets/bad-yaml/DESCRIPTION delete mode 100644 tests/testthat/assets/bad-yaml/_pkgdown.yml diff --git a/tests/testthat/_snaps/package.md b/tests/testthat/_snaps/package.md index 3f6976305..08b1e5e6f 100644 --- a/tests/testthat/_snaps/package.md +++ b/tests/testthat/_snaps/package.md @@ -23,10 +23,10 @@ # read_meta() errors gracefully if _pkgdown.yml failed to parse Code - as_pkgdown(test_path("assets/bad-yaml")) + as_pkgdown(pkg$src_path) Condition Error in `as_pkgdown()`: - ! Could not parse config file at 'assets/bad-yaml/_pkgdown.yml'. + ! Could not parse config file at '/_pkgdown.yml'. Caused by error in `yaml.load()`: ! Scanner error: mapping values are not allowed in this context at line 2, column 8 diff --git a/tests/testthat/assets/bad-yaml/DESCRIPTION b/tests/testthat/assets/bad-yaml/DESCRIPTION deleted file mode 100644 index 2329d143b..000000000 --- a/tests/testthat/assets/bad-yaml/DESCRIPTION +++ /dev/null @@ -1,3 +0,0 @@ -Package: kittens -Authors@R: person("Hadley", "Wickham", , "hadley@rstudio.com", role = c("aut", "cre")), -VignetteBuilder: knitr diff --git a/tests/testthat/assets/bad-yaml/_pkgdown.yml b/tests/testthat/assets/bad-yaml/_pkgdown.yml deleted file mode 100644 index 01904824d..000000000 --- a/tests/testthat/assets/bad-yaml/_pkgdown.yml +++ /dev/null @@ -1,6 +0,0 @@ -url: https://pkgdown.r-lib.org - title: Build websites for R packages - -authors: - Jay Hesselberth: - href: https://hesselberthlab.org diff --git a/tests/testthat/test-package.R b/tests/testthat/test-package.R index ffd061f51..a016207a9 100644 --- a/tests/testthat/test-package.R +++ b/tests/testthat/test-package.R @@ -84,8 +84,14 @@ test_that("titles don't get autolinked code", { }) test_that("read_meta() errors gracefully if _pkgdown.yml failed to parse", { + pkg <- local_pkgdown_site() + write_lines(path = path(pkg$src_path, "_pkgdown.yml"), c( + "url: https://pkgdown.r-lib.org", + " title: Build websites for R packages" + )) expect_snapshot( + as_pkgdown(pkg$src_path), error = TRUE, - as_pkgdown(test_path("assets/bad-yaml")) + transform = function(x) gsub(pkg$src_path, "", x, fixed = TRUE) ) })