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.

Sign up for GitHub

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

Jump to bottom

Update OOB errors #1737

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Update OOB errors #1737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4664f27
Use common header method in subscript errors
lionel- Oct 21, 2022
d78350c
Use "actual vs expected" phrasing in OOB errors
lionel- Oct 21, 2022
5ed99f0
Use standard subscript error header in OOB errors
lionel- Oct 21, 2022
3002741
Simplify non-consecutive errors
lionel- Oct 21, 2022
a1e7bbf
Mention larger locations in a cross bullet
lionel- Oct 21, 2022
ad93a63
Use "can't" phrasing in unknown named location errors
lionel- Oct 21, 2022
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
4 changes: 1 addition & 3 deletions NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,7 @@ S3method(cnd_header,vctrs_error_matches_remaining)
S3method(cnd_header,vctrs_error_names_cannot_be_dot_dot)
S3method(cnd_header,vctrs_error_names_cannot_be_empty)
S3method(cnd_header,vctrs_error_names_must_be_unique)
S3method(cnd_header,vctrs_error_subscript_oob)
S3method(cnd_header,vctrs_error_subscript_size)
S3method(cnd_header,vctrs_error_subscript_type)
S3method(cnd_header,vctrs_error_subscript)
S3method(diff,vctrs_vctr)
S3method(duplicated,vctrs_sclr)
S3method(duplicated,vctrs_vctr)
Expand Down
86 changes: 28 additions & 58 deletions R/subscript-loc.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,34 +487,12 @@ stop_subscript_oob <- function(i,
)
}

#' @export
cnd_header.vctrs_error_subscript_oob <- function(cnd, ...) {
if (cnd_subscript_oob_non_consecutive(cnd)) {
return(cnd_header_vctrs_error_subscript_oob_non_consecutive(cnd, ...))
}

elt <- cnd_subscript_element(cnd)
action <- cnd_subscript_action(cnd)
type <- cnd_subscript_type(cnd)

if (action %in% c("rename", "relocate") || type == "character") {
glue::glue("Can't {action} {elt[[2]]} that don't exist.")
} else {
glue::glue("Can't {action} {elt[[2]]} past the end.")
}
}

#' @export
cnd_body.vctrs_error_subscript_oob <- function(cnd, ...) {
switch(cnd_subscript_type(cnd),
numeric =
if (cnd_subscript_oob_non_consecutive(cnd)) {
cnd_body_vctrs_error_subscript_oob_non_consecutive(cnd, ...)
} else {
cnd_body_vctrs_error_subscript_oob_location(cnd, ...)
},
character =
cnd_body_vctrs_error_subscript_oob_name(cnd, ...),
switch(
cnd_subscript_type(cnd),
numeric = cnd_body_vctrs_error_subscript_oob_location(cnd, ...),
character = cnd_body_vctrs_error_subscript_oob_name(cnd, ...),
abort("Internal error: subscript type can't be `logical` for OOB errors.")
)
}
Expand All @@ -533,28 +511,46 @@ cnd_body_vctrs_error_subscript_oob_location <- function(cnd, ...) {
i <- abs(i)

oob <- i[i > cnd$size]
oob_enum <- vctrs_cli_vec(oob)

n_loc <- length(oob)
n <- cnd$size
elt <- cnd_subscript_element_cli(n, cnd)

allow_extension <- cnd_subscript_oob_non_consecutive(cnd)
scalar_oob <- length(oob) == 1

arg <- cnd_subscript_arg(cnd)

if (scalar_oob) {
arg <- arg
not <- glue::glue(", not {oob}")
oob_line <- NULL
} else {
arg <- glue::glue("Locations in {arg}")
not <- ""
oob_enum <- vctrs_cli_vec(oob)
oob_line <- cli::format_inline("Larger locations: {oob_enum}")
}

# TODO: Switch to `format_inline()` and format bullets lazily through rlang
cli::format_error(c(
"i" = "{cli::qty(n_loc)} Location{?s} {oob_enum} do{?esn't/n't} exist.",
"i" = "There {cli::qty(n)} {?is/are} only {elt}."
"x" = "{cli::qty(n_loc)} Location{?s} must be less than or equal to {n}{not}.",
"x" = oob_line,
"i" = "There {cli::qty(n)} {?is/are} only {elt}.",
"i" = if (allow_extension) "Extension with consecutive locations is allowed."
))
}
cnd_body_vctrs_error_subscript_oob_name <- function(cnd, ...) {
elt <- cnd_subscript_element(cnd, capital = TRUE)
elt <- cnd_subscript_element(cnd, capital = FALSE)
elt_cap <- cnd_subscript_element(cnd, capital = TRUE)
oob <- cnd$i[!cnd$i %in% cnd$names]
oob_enum <- enumerate(glue::backtick(oob))

format_error_bullets(c(
x = glue::glue(ngettext(
length(oob),
"{elt[[1]]} {oob_enum} doesn't exist.",
"{elt[[2]]} {oob_enum} don't exist."
"Can't find {elt[[1]]} {oob_enum}.",
"Can't find {elt[[2]]} {oob_enum}.",
))
))
}
Expand All @@ -577,32 +573,6 @@ stop_location_oob_non_consecutive <- function(i,
)
}

cnd_header_vctrs_error_subscript_oob_non_consecutive <- function(cnd, ...) {
action <- cnd_subscript_action(cnd)
elt <- cnd_subscript_element(cnd)
glue::glue("Can't {action} {elt[[2]]} beyond the end with non-consecutive locations.")
}
cnd_body_vctrs_error_subscript_oob_non_consecutive <- function(cnd, ...) {
i <- sort(cnd$i)
i <- i[i > cnd$size]

non_consecutive <- i[c(TRUE, diff(i) != 1L)]

arg <- append_arg("Subscript", cnd$subscript_arg)
if (length(non_consecutive) == 1) {
x_line <- glue::glue("{arg} contains non-consecutive location {non_consecutive}.")
} else {
non_consecutive <- ensure_full_stop(enumerate(non_consecutive))
x_line <- glue::glue("{arg} contains non-consecutive locations {non_consecutive}")
}

glue_data_bullets(
cnd,
i = "Input has size {size}.",
x = x_line
)
}

cnd_subscript_oob_non_consecutive <- function(cnd) {
out <- cnd$subscript_oob_non_consecutive %||% FALSE
check_bool(out)
Expand Down
6 changes: 1 addition & 5 deletions R/subscript.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ new_error_subscript_type <- function(i,
}

#' @export
cnd_header.vctrs_error_subscript_type <- function(cnd) {
cnd_header.vctrs_error_subscript <- function(cnd) {
arg <- cnd[["subscript_arg"]]
if (is_subscript_arg(arg)) {
with <- glue::glue(" with {format_subscript_arg(arg)}")
Expand Down Expand Up @@ -233,10 +233,6 @@ new_error_subscript_size <- function(i,
...
)
}
#' @export
cnd_header.vctrs_error_subscript_size <- function(cnd, ...) {
cnd_header.vctrs_error_subscript_type(cnd, ...)
}

new_error_subscript2_type <- function(i,
numeric,
Expand Down
12 changes: 6 additions & 6 deletions tests/testthat/_snaps/conditions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,24 @@
Output
<error/vctrs_error_subscript_oob>
Error in `vec_slice()`:
! Can't subset elements that don't exist.
x Element `foo` doesn't exist.
! Can't subset elements.
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need "Can't subset elements" in these three errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that this is this header that carries the arg information, if any. That will be the case when called from select() and tibble's subset methods.

x Can't find element `foo`.
Code
(expect_error(with_subscript_data(vec_slice(set_names(letters), "foo"), quote(
foo)), class = "vctrs_error_subscript_oob"))
Output
<error/vctrs_error_subscript_oob>
Error in `vec_slice()`:
! Can't subset elements that don't exist.
x Element `foo` doesn't exist.
! Can't subset elements with `foo`.
x Can't find element `foo`.
Code
(expect_error(with_subscript_data(vec_slice(set_names(letters), "foo"), quote(
foo(bar))), class = "vctrs_error_subscript_oob"))
Output
<error/vctrs_error_subscript_oob>
Error in `vec_slice()`:
! Can't subset elements that don't exist.
x Element `foo` doesn't exist.
! Can't subset elements with `foo(bar)`.
x Can't find element `foo`.

# scalar type errors are informative

Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/_snaps/error-call.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@
Output
<error/vctrs_error_subscript_oob>
Error in `my_function()`:
! Can't subset elements past the end.
i Location 10 doesn't exist.
! Can't subset elements with `10`.
x Location must be less than or equal to 2, not 10.
Copy link
Member

Choose a reason for hiding this comment

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

Could this be one line like Can't find location 10; input is length 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is slightly problematic because these errors are used for subsetting across tibble rows and cols. So it's hard to find generic formulations that work well for all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could say "input has 2 elements" or "input has 3 rows". This is how we currently phrase it in an additional info bullet, but I guess you're saying this information should be closer?

Another difficult aspect is how to handle the case where multiple locations are not found without creating a too long line. To address this, I try to consistently mention multiple locations in their own bullet in this PR. But then it's hard to keep the length info close.

i There are only 2 elements.

---
Expand Down
12 changes: 6 additions & 6 deletions tests/testthat/_snaps/slice-assign.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@
Output
<error/vctrs_error_subscript_oob>
Error:
! Can't assign to elements past the end.
i Location 5 doesn't exist.
! Can't assign elements.
x Location must be less than or equal to 3, not 5.
i There are only 3 elements.
Code
(expect_error(vec_assign(1:3, "foo", 10), "unnamed vector"))
Expand All @@ -52,17 +52,17 @@
Output
<error/vctrs_error_subscript_oob>
Error:
! Can't negate elements past the end.
i Location 100 doesn't exist.
! Can't negate elements.
x Location must be less than or equal to 26, not 100.
i There are only 26 elements.
Code
(expect_error(vec_assign(set_names(letters), "foo", "bar"), class = "vctrs_error_subscript_oob")
)
Output
<error/vctrs_error_subscript_oob>
Error:
! Can't assign to elements that don't exist.
x Element `foo` doesn't exist.
! Can't assign elements.
x Can't find element `foo`.

# must assign with proper negative locations

Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/_snaps/slice-chop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,8 @@
Output
<error/vctrs_error_subscript_oob>
Error:
! Can't subset elements past the end.
i Location 3 doesn't exist.
! Can't subset elements.
x Location must be less than or equal to 2, not 3.
i There are only 2 elements.

---
Expand Down
30 changes: 16 additions & 14 deletions tests/testthat/_snaps/slice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,16 @@
Output
<error/vctrs_error_subscript_oob>
Error in `vec_slice()`:
! Can't subset elements past the end.
i Location 3 doesn't exist.
! Can't subset elements with `i`.
x Location must be less than or equal to 2, not 3.
i There are only 2 elements.
Code
(expect_error(vec_slice(1:2, -3L), class = "vctrs_error_subscript_oob"))
Output
<error/vctrs_error_subscript_oob>
Error in `vec_slice()`:
! Can't negate elements past the end.
i Location 3 doesn't exist.
! Can't negate elements with `i`.
x Location must be less than or equal to 2, not 3.
i There are only 2 elements.

# can slice with double indices
Expand Down Expand Up @@ -79,17 +79,18 @@
vec_slice(c(bar = 1), "foo")
Condition
Error in `vec_slice()`:
! Can't subset elements that don't exist.
x Element `foo` doesn't exist.
! Can't subset elements with `i`.
x Can't find element `foo`.

---

Code
vec_slice(letters, c(100, 1000))
Condition
Error in `vec_slice()`:
! Can't subset elements past the end.
i Locations 100 and 1000 don't exist.
! Can't subset elements with `i`.
x Locations must be less than or equal to 26.
x Larger locations: 100 and 1000
i There are only 26 elements.

---
Expand All @@ -98,8 +99,9 @@
vec_slice(letters, c(1, 100:103, 2, 104:110))
Condition
Error in `vec_slice()`:
! Can't subset elements past the end.
i Locations 100, 101, 102, ..., 109, and 110 don't exist.
! Can't subset elements with `i`.
x Locations must be less than or equal to 26.
x Larger locations: 100, 101, 102, ..., 109, and 110
i There are only 26 elements.

---
Expand All @@ -108,17 +110,17 @@
vec_slice(set_names(letters), c("foo", "bar"))
Condition
Error in `vec_slice()`:
! Can't subset elements that don't exist.
x Elements `foo` and `bar` don't exist.
! Can't subset elements with `i`.
x Can't find elements `foo` and `bar`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a case where chained errors might fit better because there are multiple bullets "can't / must" bullets describing a problem at different levels.

Same for the "must" cross bullet above.


---

Code
vec_slice(set_names(letters), toupper(letters))
Condition
Error in `vec_slice()`:
! Can't subset elements that don't exist.
x Elements `A`, `B`, `C`, `D`, `E`, etc. don't exist.
! Can't subset elements with `i`.
x Can't find elements `A`, `B`, `C`, `D`, `E`, etc..

# vec_init() validates `n`

Expand Down
Loading