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

have a valid hyperlink in all error messages #2415

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions R/build-articles.R
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ data_articles_index <- function(pkg = ".") {
if (length(missing) > 0) {
cli::cli_abort(
"{length(missing)} vignette{?s} missing from index in \\
{pkgdown_config_href({pkg$src_path})}: {.val {missing}}.",
{.file {pkgdown_config_relpath(pkg)}}: {.val {missing}}.",
call = caller_env()
)
}
Expand All @@ -392,7 +392,7 @@ data_articles_index_section <- function(section, pkg) {
}

# Match topics against any aliases
in_section <- select_vignettes(section$contents, pkg$vignettes)
in_section <- select_vignettes(section$contents, pkg$vignettes, pkg = pkg)
section_vignettes <- pkg$vignettes[in_section, ]
contents <- tibble::tibble(
name = section_vignettes$name,
Expand All @@ -411,13 +411,13 @@ data_articles_index_section <- function(section, pkg) {

# Quick hack: create the same structure as for topics so we can use
# the existing select_topics()
select_vignettes <- function(match_strings, vignettes) {
select_vignettes <- function(match_strings, vignettes, pkg) {
topics <- tibble::tibble(
name = vignettes$name,
alias = as.list(vignettes$name),
internal = FALSE
)
select_topics(match_strings, topics)
select_topics(match_strings, topics, pkg = pkg)
}

default_articles_index <- function(pkg = ".") {
Expand Down
4 changes: 2 additions & 2 deletions R/build-reference-index.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ data_reference_index_rows <- function(section, index, pkg) {

if (has_name(section, "contents")) {
check_all_characters(section$contents, index, pkg)
topics <- section_topics(section$contents, pkg$topics, pkg$src_path)
topics <- section_topics(section$contents, pkg$topics, pkg$src_path, pkg)

names <- topics$name
topics$name <- NULL
Expand Down Expand Up @@ -136,7 +136,7 @@ check_missing_topics <- function(rows, pkg, error_call = caller_env()) {
cli::cli_abort(c(
"All topics must be included in reference index",
"x" = "Missing topics: {pkg$topics$name[missing]}",
i = "Either add to {pkgdown_config_href({pkg$src_path})} or use @keywords internal"
i = "Either add to {.file {pkgdown_config_relpath(pkg)}} or use @keywords internal"
),
call = error_call)
}
Expand Down
2 changes: 1 addition & 1 deletion R/check.R
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ check_pkgdown <- function(pkg = ".") {
data_reference_index(pkg)

cli::cli_inform(c(
"v" = "No problems found in {pkgdown_config_href({pkg$src_path})}"
"v" = "No problems found in {.file {pkgdown_config_relpath(pkg)}}"
))
}
2 changes: 1 addition & 1 deletion R/navbar.R
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ navbar_articles <- function(pkg = ".") {
menu$articles <- menu_link(tr_("Articles"), "articles/index.html")
} else {
sections <- lapply(navbar, function(section) {
vig <- pkg$vignettes[select_vignettes(section$contents, pkg$vignettes), , drop = FALSE]
vig <- pkg$vignettes[select_vignettes(section$contents, pkg$vignettes, pkg), , drop = FALSE]
vig <- vig[vig$name != pkg$package, , drop = FALSE]
c(
if (!is.null(section$navbar)) list(menu_spacer(), menu_text(section$navbar)),
Expand Down
6 changes: 0 additions & 6 deletions R/package.R
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,6 @@ pkgdown_config_path <- function(path) {
)
)
}
pkgdown_config_href <- function(path) {
cli::style_hyperlink(
text = "_pkgdown.yml",
url = paste0("file://", pkgdown_config_path(path))
)
}

read_meta <- function(path) {
path <- pkgdown_config_path(path)
Expand Down
32 changes: 17 additions & 15 deletions R/topics.R
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
# @return An integer vector giving selected topics
select_topics <- function(match_strings, topics, check = FALSE) {
select_topics <- function(match_strings, topics, pkg = ".", check = FALSE) {
n <- nrow(topics)
if (length(match_strings) == 0) {
return(integer())
}

indexes <- purrr::map(match_strings, match_eval, env = match_env(topics))

indexes <- purrr::map(
match_strings,
function(x) match_eval(x, env = match_env(topics), pkg = pkg)
)
# If none of the specified topics have a match, return no topics
if (purrr::every(indexes, is_empty)) {
if (check) {
cli::cli_abort(
"No topics matched in {.file _pkgdown.yml}. No topics selected.",
"No topics matched in {.file {pkgdown_config_relpath(pkg)}}. No topics selected.",
call = caller_env()
)
}
Expand All @@ -20,7 +22,7 @@ select_topics <- function(match_strings, topics, check = FALSE) {

no_match <- match_strings[purrr::map_lgl(indexes, rlang::is_empty)]
if (check && length(no_match) > 0) {
topic_must("match a function or concept", toString(no_match))
topic_must("match a function or concept", toString(no_match), pkg = pkg)
}

indexes <- purrr::discard(indexes, is_empty)
Expand Down Expand Up @@ -134,7 +136,7 @@ match_env <- function(topics) {
}


match_eval <- function(string, env) {
match_eval <- function(string, env, pkg = ".") {
# Early return in case string already matches symbol
if (env_has(env, string)) {
val <- env[[string]]
Expand All @@ -145,7 +147,7 @@ match_eval <- function(string, env) {

expr <- tryCatch(parse_expr(string), error = function(e) NULL)
if (is.null(expr)) {
topic_must("be valid R code", string)
topic_must("be valid R code", string, pkg = pkg)
}

if (is_string(expr) || is_symbol(expr)) {
Expand All @@ -154,42 +156,42 @@ match_eval <- function(string, env) {
if (is.integer(val)) {
val
} else {
topic_must("be a known topic name or alias", string)
topic_must("be a known topic name or alias", string, pkg = pkg)
}
} else if (is_call(expr, "::")) {
name <- paste0(expr[[2]], "::", expr[[3]])
val <- env_get(env, name, default = NULL)
if (is.integer(val)) {
val
} else {
topic_must("be a known topic name or alias", string)
topic_must("be a known topic name or alias", string, pkg = pkg)
}
} else if (is_call(expr)) {
tryCatch(
eval(expr, env),
error = function(e) {
topic_must("be a known selector function", string, parent = e)
topic_must("be a known selector function", string, parent = e, pkg = pkg)
}
)
} else {
topic_must("be a string or function call", string)
topic_must("be a string or function call", string, pkg = pkg)
}
}

topic_must <- function(message, topic, ..., call = NULL) {
topic_must <- function(message, topic, ..., call = NULL, pkg = ".") {
cli::cli_abort(
"In {.file _pkgdown.yml}, topic must {message}, not {.val {topic}}.",
"In {.file {pkgdown_config_relpath(pkg)}}, topic must {message}, not {.val {topic}}.",
...,
call = call
)
}

section_topics <- function(match_strings, topics, src_path) {
section_topics <- function(match_strings, topics, src_path, pkg = ".") {
# Add rows for external docs
ext_strings <- match_strings[grepl("::", match_strings, fixed = TRUE)]
topics <- rbind(topics, ext_topics(ext_strings))

selected <- topics[select_topics(match_strings, topics), , ]
selected <- topics[select_topics(match_strings, topics, pkg), , ]

tibble::tibble(
name = selected$name,
Expand Down
10 changes: 7 additions & 3 deletions R/utils-fs.R
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,13 @@ path_pkgdown <- function(...) {
system_file(..., package = "pkgdown")
}

# To avoid problems in tests, default to _pkgdown.yml
pkgdown_config_relpath <- function(pkg) {
pkg <- as_pkgdown(pkg)
config_path <- pkgdown_config_path(pkg$src_path)
tryCatch({
pkg <- as_pkgdown(pkg)
config_path <- pkgdown_config_path(pkg$src_path)

fs::path_rel(config_path, pkg$src_path)
fs::path_rel(config_path, pkg$src_path)
},
error = function(e) fs::path("_pkgdown.yml"))
}
6 changes: 3 additions & 3 deletions tests/testthat/_snaps/build-reference-index.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@
Error:
! All topics must be included in reference index
x Missing topics: c, e, and ?
i Either add to _pkgdown.yml or use @keywords internal
i Either add to 'pkgdown/_pkgdown.yml' or use @keywords internal

# errors well when a content entry is empty

i In index: 1.
Caused by error in `.f()`:
! Item 2 in section 1 is empty.
x Delete the empty line or add function name to reference in '_pkgdown.yml'.
x Delete the empty line or add function name to reference in 'pkgdown/_pkgdown.yml'.

# errors well when a content entry is not a character

Expand All @@ -71,7 +71,7 @@
i In index: 1.
Caused by error in `.f()`:
! Item 2 in section 1 must be a character.
x You might need to add '' around e.g. - 'N' or - 'off' to reference in '_pkgdown.yml'.
x You might need to add '' around e.g. - 'N' or - 'off' to reference in 'pkgdown/_pkgdown.yml'.

# errors well when a content entry refers to a not installed package

Expand Down
6 changes: 3 additions & 3 deletions tests/testthat/_snaps/check.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@
Error in `check_pkgdown()`:
! All topics must be included in reference index
x Missing topics: ?
i Either add to _pkgdown.yml or use @keywords internal
i Either add to 'pkgdown/_pkgdown.yml' or use @keywords internal

# fails if article index incomplete

Code
check_pkgdown(pkg)
Condition
Error in `check_pkgdown()`:
! 2 vignettes missing from index in _pkgdown.yml: "articles/nested" and "width".
! 2 vignettes missing from index in '_pkgdown.yml': "articles/nested" and "width".

# informs if everything is ok

Code
check_pkgdown(pkg)
Message
v No problems found in _pkgdown.yml
v No problems found in 'pkgdown/_pkgdown.yml'

2 changes: 1 addition & 1 deletion tests/testthat/_snaps/render.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# check_bootswatch_theme() works

Can't find Bootswatch theme "paper" (template.bootswatch) for Bootstrap version "4" (template.bootstrap).
x Edit settings in '_pkgdown.yml'
x Edit settings in 'pkgdown/_pkgdown.yml'

# capture data_template()

Expand Down
10 changes: 9 additions & 1 deletion tests/testthat/test-topics.R
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ test_that("full topic selection process works", {
pkg <- local_pkgdown_site(test_path("assets/reference"))

# can mix local and remote
out <- section_topics(c("a", "base::mean"), pkg$topics, pkg$src_path)
out <- section_topics(c("a", "base::mean"), pkg$topics, pkg$src_path, pkg)
expect_equal(unname(out$name), c("a", "base::mean"))

# concepts and keywords work
Expand All @@ -163,6 +163,14 @@ test_that("full topic selection process works", {
pkg$src_path
)
expect_equal(unname(out$name), c("b", "a"))
# concepts and keywords error with correct path
expect_error(section_topics(
c("foo2"),
pkg$topics,
pkg$src_path,
pkg
),
regexp = "pkgdown/_pkgdown.yml")
})

test_that("an unmatched selection with a matched selection does not select everything", {
Expand Down
Loading