Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start writing config pluck helpers #2514

Merged
merged 12 commits into from
May 10, 2024
66 changes: 35 additions & 31 deletions R/build-home-index.R
Original file line number Diff line number Diff line change
Expand Up @@ -96,61 +96,65 @@ data_home_sidebar <- function(pkg = ".", call = caller_env()) {
return(sidebar_html)
}

# compute any custom component
components <- pkg$meta$home$sidebar$components
custom <- pkg$meta$home$sidebar$components
sidebar_custom <- unwrap_purrr_error(purrr::map(
set_names(names2(custom)),
function(comp) {
data_home_component(
custom[[comp]],
error_pkg = pkg,
error_path = paste0("home.sidebar.components.", comp),
error_call = call
)
}
))
sidebar_components <- utils::modifyList(sidebar_components, sidebar_custom)

sidebar_components <- utils::modifyList(
config_check_list(
sidebar_components,
unwrap_purrr_error(purrr::map2(
components,
names(components),
data_home_component,
pkg = pkg,
call = call
)) %>%
set_names(names(components))
)

check_yaml_has(
setdiff(sidebar_structure, names(sidebar_components)),
where = c("home", "sidebar", "components"),
pkg = pkg,
call = call
sidebar_structure,
error_pkg = pkg,
error_path = "home.sidebar.components",
error_call = call
)

sidebar_final_components <- purrr::compact(
sidebar_components[sidebar_structure]
)

paste0(sidebar_final_components, collapse = "\n")

}

# Update sidebar-configuration.Rmd if this changes
default_sidebar_structure <- function() {
c("links", "license", "community", "citation", "authors", "dev")
}

data_home_component <- function(component, component_name, pkg, call = caller_env()) {

check_yaml_has(
setdiff(c("title", "text"), names(component)),
where = c("home", "sidebar", "components", component_name),
pkg = pkg,
call = call
)

sidebar_section(
data_home_component <- function(component,
error_pkg,
error_path,
error_call = caller_env()) {
title <- config_check_string(
component$title,
bullets = markdown_text_block(component$text)
error_pkg = error_pkg,
error_path = paste0(error_path, ".title"),
error_call = error_call
)
text <- config_check_string(
component$text,
error_pkg = error_pkg,
error_path = paste0(error_path, ".text"),
error_call = error_call
)

sidebar_section(title, bullets = markdown_text_block(text))
}

data_home_sidebar_links <- function(pkg = ".") {
pkg <- as_pkgdown(pkg)

repo <- cran_link(pkg$package)
links <- purrr::pluck(pkg, "meta", "home", "links")
links <- config_pluck_character(pkg, "home.links")

links <- c(
link_url(sprintf(tr_("View on %s"), repo$repo), repo$url),
Expand Down
125 changes: 103 additions & 22 deletions R/config.R
Original file line number Diff line number Diff line change
@@ -1,45 +1,126 @@
check_yaml_has <- function(missing, where, pkg, call = caller_env()) {
if (length(missing) == 0) {
return()
}
config_pluck <- function(pkg, path, default = NULL) {
check_string(path, allow_empty = FALSE, .internal = TRUE)

missing_components <- lapply(missing, function(x) c(where, x))
msg_flds <- purrr::map_chr(missing_components, paste, collapse = ".")
where <- strsplit(path, ".", fixed = TRUE)[[1]]
purrr::pluck(pkg$meta, !!!where, .default = default)
}

config_abort(
pkg,
"Can't find {cli::qty(missing)} component{?s} {.field {msg_flds}}.",
call = call
config_pluck_character <- function(pkg,
path,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be called config_pluck_chr()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the string one, config_pluck_str()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are more matches to rlang's check_ helpers than purrr's, because they're about selecting single values (that are sometimes vectors).

default = character(),
call = caller_env()) {
x <- config_pluck(pkg, path, default)
config_check_character(
x,
error_path = path,
error_pkg = pkg,
error_call = call
)
}

yaml_character <- function(pkg, where) {
x <- purrr::pluck(pkg$meta, !!!where)
config_pluck_string <- function(pkg,
path,
default = "",
call = caller_env()) {
x <- config_pluck(pkg, path, default)
config_check_string(
x,
error_path = path,
error_pkg = pkg,
error_call = call
)
}

# checks ---------------------------------------------------------------------

if (identical(x, list()) || is.null(x)) {
config_check_character <- function(x,
error_pkg,
error_path,
error_call = caller_env()) {
if (is.character(x)) {
x
} else if (identical(x, list())) {
character()
} else if (is.character(x)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know 😄 But I think it must be some yaml special case where we want to handle an empty list.

} else {
config_abort_type(
must_be = "a character vector",
not = x,
error_pkg = error_pkg,
error_path = error_path,
error_call = error_call
)
}
}

config_check_string <- function(x,
error_pkg,
error_path,
error_call = caller_env()) {

if (is_string(x)) {
x
} else {
path <- paste0(where, collapse = ".")
config_abort_type(
must_be = "a string",
not = x,
error_pkg = error_pkg,
error_path = error_path,
error_call = error_call
)
}
}

config_abort_type <- function(must_be, not, error_pkg, error_path, error_call) {
not_str <- obj_type_friendly(not)
config_abort(
error_pkg,
"{.field {error_path}} must be {must_be}, not {not_str}.",
call = error_call
)

}

config_check_list <- function(x,
names = NULL,
error_pkg,
error_path,
error_call = caller_env()) {
if (is_list(x)) {
if (!is.null(names) && !all(has_name(x, names))) {
missing <- setdiff(names, names(x))
Copy link
Collaborator

@maelle maelle May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on my confusion in the tests before I read the error snapshots:

Suggested change
if (!is.null(names) && !all(has_name(x, names))) {
some_required_components_absent <- (!is.null(names) && !all(has_name(x, names)))
if (some_required_components_absent) {

names is maybe not specific enough, could it be "required" or so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe has_names?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has_required? names is a bit vague

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried has_names and I think it's pretty clear when read in context.

config_abort(
error_pkg,
c(
"{.field {error_path}} must have components {.str {names}}.",
"{length(missing)} missing component{?s}: {.str {missing}}."
),
call = error_call
)
} else {
x
}
} else {
not <- obj_type_friendly(x)
config_abort(
pkg,
"{.field {path}} must be a character vector.",
call = caller_env()
error_pkg,
"{.field {error_path}} must be a list, not {not}.",
call = error_call
)
}
}

# generic error ---------------------------------------------------------------

config_abort <- function(pkg,
message,
...,
call = caller_env(),
.envir = caller_env()) {

edit <- cli::format_inline("Edit {config_path(pkg)} to fix the problem.")

cli::cli_abort(
c(
message,
i = "Edit {config_path(pkg)} to fix the problem."
),
c(message, i = edit),
...,
call = call,
.envir = .envir
Expand Down
2 changes: 1 addition & 1 deletion R/init.R
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ init_site <- function(pkg = ".") {

copy_assets <- function(pkg = ".") {
pkg <- as_pkgdown(pkg)
template <- purrr::pluck(pkg$meta, "template", .default = list())
template <- config_pluck(pkg, "template")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default to NULL for consistency; there shouldn't be any functional difference because NULL$foo and list()$foo both return NULL.


# pkgdown assets
if (!identical(template$default_assets, FALSE)) {
Expand Down
8 changes: 4 additions & 4 deletions R/navbar.R
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
data_navbar <- function(pkg = ".", depth = 0L) {
pkg <- as_pkgdown(pkg)

navbar <- purrr::pluck(pkg, "meta", "navbar")
navbar <- config_pluck(pkg, "navbar")

style <- navbar_style(
navbar = navbar,
Expand Down Expand Up @@ -36,7 +36,7 @@ navbar_structure <- function() {
}

navbar_links <- function(pkg, depth = 0L) {
navbar <- purrr::pluck(pkg, "meta", "navbar")
navbar <- config_pluck(pkg, "navbar")

# Combine default components with user supplied
components <- navbar_components(pkg)
Expand All @@ -47,11 +47,11 @@ navbar_links <- function(pkg, depth = 0L) {
# Combine default structure with user supplied
pkg$meta$navbar$structure <- modify_list(navbar_structure(), pkg$meta$navbar$structure)
right_comp <- intersect(
yaml_character(pkg, c("navbar", "structure", "right")),
config_pluck_character(pkg, "navbar.structure.right"),
names(components)
)
left_comp <- intersect(
yaml_character(pkg, c("navbar", "structure", "left")),
config_pluck_character(pkg, "navbar.structure.left"),
names(components)
)
# Backward compatibility
Expand Down
4 changes: 2 additions & 2 deletions R/render.R
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ data_template <- function(pkg = ".", depth = 0L) {
css = path_first_existing(pkg$src_path, "pkgdown", "extra.css"),
js = path_first_existing(pkg$src_path, "pkgdown", "extra.js")
)
out$includes <- purrr::pluck(pkg, "meta", "template", "includes", .default = list())
out$yaml <- purrr::pluck(pkg, "meta", "template", "params", .default = list())
out$includes <- config_pluck(pkg, "template.includes")
out$yaml <- config_pluck(pkg, "template.params")
# Force inclusion so you can reliably refer to objects inside yaml
# in the mustache templates
out$yaml$.present <- TRUE
Expand Down
6 changes: 3 additions & 3 deletions R/theme.R
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ bs_theme <- function(pkg = ".") {
bs_theme_rules <- function(pkg) {
paths <- path_pkgdown("BS5", "assets", "pkgdown.scss")

theme <- purrr::pluck(pkg, "meta", "template", "theme", .default = "arrow-light")
theme <- config_pluck_string(pkg, "template.theme", default = "arrow-light")
theme_path <- path_pkgdown("highlight-styles", paste0(theme, ".scss"))
if (!file_exists(theme_path)) {
cli::cli_abort(c(
Expand All @@ -66,8 +66,8 @@ bs_theme_rules <- function(pkg) {
}
paths <- c(paths, theme_path)

package <- purrr::pluck(pkg, "meta", "template", "package")
if (!is.null(package)) {
package <- config_pluck_string(pkg, "template.package")
if (package != "") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (package != "") {
if (nzchar(package)) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I find that style hard to read

package_extra <- path_package_pkgdown("extra.scss", package, pkg$bs_version)
if (file_exists(package_extra)) {
paths <- c(paths, package_extra)
Expand Down
10 changes: 6 additions & 4 deletions tests/testthat/_snaps/build-home-index.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@
data_home_sidebar(pkg)
Condition
Error:
! Can't find component home.sidebar.components.fancy.
! home.sidebar.components must have components "fancy".
1 missing component: "fancy".
i Edit _pkgdown.yml to fix the problem.

---
Expand All @@ -84,7 +85,8 @@
data_home_sidebar(pkg)
Condition
Error:
! Can't find components home.sidebar.components.fancy and home.sidebar.components.cool.
! home.sidebar.components must have components "fancy" and "cool".
2 missing components: "fancy" and "cool".
i Edit _pkgdown.yml to fix the problem.

---
Expand All @@ -93,7 +95,7 @@
data_home_sidebar(pkg)
Condition
Error:
! Can't find component home.sidebar.components.fancy.title.
! home.sidebar.components.fancy.title must be a string, not `NULL`.
i Edit _pkgdown.yml to fix the problem.

---
Expand All @@ -102,6 +104,6 @@
data_home_sidebar(pkg)
Condition
Error:
! Can't find components home.sidebar.components.fancy.title and home.sidebar.components.fancy.text.
! home.sidebar.components.fancy.title must be a string, not `NULL`.
i Edit _pkgdown.yml to fix the problem.

20 changes: 15 additions & 5 deletions tests/testthat/_snaps/config.md
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
# check_yaml_has produces informative errors
# config_check_list gives informative errors

Code
check_yaml_has("x", where = "a", pkg = pkg)
config_check_list(1, "x", error_pkg = pkg, error_path = "path")
Condition
Error:
! Can't find component a.x.
! path must be a list, not the number 1.
i Edit _pkgdown.yml to fix the problem.
Code
check_yaml_has(c("x", "y"), where = "a", pkg = pkg)
config_check_list(list(x = 1, y = 1), c("y", "z"), error_pkg = pkg, error_path = "path")
Condition
Error:
! Can't find components a.x and a.y.
! path must have components "y" and "z".
1 missing component: "z".
i Edit _pkgdown.yml to fix the problem.

# config_pluck_character generates informative error

Code
config_pluck_character(pkg, "x")
Condition
Error:
! x must be a character vector, not the number 1.
i Edit _pkgdown.yml to fix the problem.

2 changes: 1 addition & 1 deletion tests/testthat/_snaps/navbar.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@
data_navbar(pkg)
Condition
Error in `navbar_links()`:
! navbar.structure.left must be a character vector.
! navbar.structure.left must be a character vector, not the number 1.
i Edit _pkgdown.yml to fix the problem.

# data_navbar() errors with bad left/right
Expand Down
Loading
Loading