-
Notifications
You must be signed in to change notification settings - Fork 417
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
Pass calls for improved error messages #1340
Changes from 21 commits
d1c9e74
6859630
d1c71f8
d00ddf3
429a4b6
bff5cbd
be34bd3
e330efb
d3ddf32
c973a99
4c1391b
f53da26
db0ee85
222a765
ca30c5a
8d52e9b
b82f39d
14f663c
22a10cb
ed7f7b2
80e50c1
a8828f0
16467e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -444,7 +444,8 @@ pivot_wider_spec <- function(data, | |||||
rows, | ||||||
values, | ||||||
unused, | ||||||
.name_repair = names_repair | ||||||
.name_repair = names_repair, | ||||||
.error_call = current_env() | ||||||
)) | ||||||
|
||||||
reconstruct_tibble(input, out) | ||||||
|
@@ -525,17 +526,19 @@ build_wider_spec <- function(data, | |||||
build_wider_id_cols_expr <- function(data, | ||||||
id_cols = NULL, | ||||||
names_from = name, | ||||||
values_from = value) { | ||||||
values_from = value, | ||||||
call = caller_env()) { | ||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
# TODO: Use `allow_rename = FALSE`. | ||||||
# Requires https://github.com/r-lib/tidyselect/issues/225. | ||||||
names_from_cols <- names(tidyselect::eval_select(enquo(names_from), data)) | ||||||
values_from_cols <- names(tidyselect::eval_select(enquo(values_from), data)) | ||||||
names_from_cols <- names(tidyselect::eval_select(enquo(names_from), data, error_call = call)) | ||||||
values_from_cols <- names(tidyselect::eval_select(enquo(values_from), data, error_call = call)) | ||||||
|
||||||
out <- select_wider_id_cols( | ||||||
data = data, | ||||||
id_cols = {{ id_cols }}, | ||||||
names_from_cols = names_from_cols, | ||||||
values_from_cols = values_from_cols | ||||||
values_from_cols = values_from_cols, | ||||||
call = caller_env() | ||||||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
) | ||||||
|
||||||
expr(c(!!!out)) | ||||||
|
@@ -544,7 +547,8 @@ build_wider_id_cols_expr <- function(data, | |||||
select_wider_id_cols <- function(data, | ||||||
id_cols = NULL, | ||||||
names_from_cols = character(), | ||||||
values_from_cols = character()) { | ||||||
values_from_cols = character(), | ||||||
call = caller_env()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
id_cols <- enquo(id_cols) | ||||||
|
||||||
# Remove known non-id-cols so they are never selected | ||||||
|
@@ -558,38 +562,38 @@ select_wider_id_cols <- function(data, | |||||
try_fetch( | ||||||
# TODO: Use `allow_rename = FALSE`. | ||||||
# Requires https://github.com/r-lib/tidyselect/issues/225. | ||||||
id_cols <- tidyselect::eval_select(enquo(id_cols), data), | ||||||
id_cols <- tidyselect::eval_select(enquo(id_cols), data, error_call = call), | ||||||
vctrs_error_subscript_oob = function(cnd) { | ||||||
rethrow_id_cols_oob(cnd, names_from_cols, values_from_cols) | ||||||
rethrow_id_cols_oob(cnd, names_from_cols, values_from_cols, call) | ||||||
} | ||||||
) | ||||||
|
||||||
names(id_cols) | ||||||
} | ||||||
|
||||||
rethrow_id_cols_oob <- function(cnd, names_from_cols, values_from_cols) { | ||||||
rethrow_id_cols_oob <- function(cnd, names_from_cols, values_from_cols, call) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
i <- cnd[["i"]] | ||||||
|
||||||
if (!is_string(i)) { | ||||||
abort("`i` is expected to be a string.", .internal = TRUE) | ||||||
} | ||||||
|
||||||
if (i %in% names_from_cols) { | ||||||
stop_id_cols_oob(i, "names_from") | ||||||
stop_id_cols_oob(i, "names_from", call = call) | ||||||
} else if (i %in% values_from_cols) { | ||||||
stop_id_cols_oob(i, "values_from") | ||||||
stop_id_cols_oob(i, "values_from", call = call) | ||||||
} else { | ||||||
# Zap this special handler, throw the normal condition | ||||||
zap() | ||||||
} | ||||||
} | ||||||
|
||||||
stop_id_cols_oob <- function(i, arg) { | ||||||
stop_id_cols_oob <- function(i, arg, call) { | ||||||
message <- c( | ||||||
glue("`id_cols` can't select a column already selected by `{arg}`."), | ||||||
i = glue("Column `{i}` has already been selected.") | ||||||
) | ||||||
abort(message, parent = NA) | ||||||
abort(message, parent = NA, call = call) | ||||||
} | ||||||
|
||||||
# Helpers ----------------------------------------------------------------- | ||||||
|
@@ -618,7 +622,7 @@ value_summarize <- function(value, value_locs, value_name, fn, fn_name) { | |||||
x = glue("Applying `{fn_name}` resulted in a value with length {size}.") | ||||||
) | ||||||
|
||||||
abort(c(header, bullet)) | ||||||
abort(c(header, bullet), call = caller_env()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably worth making |
||||||
} | ||||||
|
||||||
value <- vec_c(!!!value) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,22 +31,22 @@ | |
#' check_pivot_spec(spec) | ||
check_pivot_spec <- function(spec) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hadley do you think this exported Or is using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it probably should |
||
if (!is.data.frame(spec)) { | ||
abort("`spec` must be a data frame.") | ||
abort("`spec` must be a data frame.", call = caller_env()) | ||
} | ||
|
||
if (!has_name(spec, ".name") || !has_name(spec, ".value")) { | ||
abort("`spec` must have `.name` and `.value` columns.") | ||
abort("`spec` must have `.name` and `.value` columns.", call = caller_env()) | ||
} | ||
|
||
if (!is.character(spec$.name)) { | ||
abort("The `.name` column of `spec` must be a character vector.") | ||
abort("The `.name` column of `spec` must be a character vector.", call = caller_env()) | ||
} | ||
if (vec_duplicate_any(spec$.name)) { | ||
abort("The `.name` column of `spec` must be unique.") | ||
abort("The `.name` column of `spec` must be unique.", call = caller_env()) | ||
} | ||
|
||
if (!is.character(spec$.value)) { | ||
abort("The `.value` column of `spec` must be a character vector.") | ||
abort("The `.value` column of `spec` must be a character vector.", call = caller_env()) | ||
} | ||
|
||
# Ensure `.name` and `.value` come first, in that order | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,6 +81,6 @@ check_replacement <- function(x, var) { | |
n <- vec_size(x) | ||
|
||
if (n != 1) { | ||
abort(glue("Replacement for `{var}` is length {n}, not length 1.")) | ||
abort(glue("Replacement for `{var}` is length {n}, not length 1."), call = caller_env()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth adding I think adding the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, agreed. I just missed updating these. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our emerging convention when passing error calls through as arguments seems to be:
Name them
call
andarg
when the function's main purpose is for some kind of input checking, i.e.vec_assert()
,check_*()
functions,abort()
, etc.Name them
error_call
anderror_arg
otherwise.Since
df_unchop()
's main purpose is unchopping, it should useerror_call
. The rest of the PR probably needs a pass to do this updating too. I'll try and mark them as I see them.We have started doing this in vctrs, tidyselect, internally in dplyr, and I've done it in ivs, so probably best to take the time to be consistent here since call improvements is what this PR is all about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I don't like this convention because it means there are 4 names it could be:
call
,.call
,error_call
, or.error_call
. I'd prefer to useerror_call
everywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we need to rename a lot of already exported functions? Isn't it a bit late to change this now?
By the way I don't consider a dotted variant to represent a different name. I see it as different syntax for the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I don't think of them as different names either, but pragmatically you have to remember which of 4 options it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in the terminal right? In IDEs you immediately see if you have a function that requires dotted arguments. It also shows you whether you need
call
orerror_call
.