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

Improve error message + tweak function signature respect error_arg for allow_rename = FALSE #358

Closed
wants to merge 6 commits into from
Closed

Improve error message + tweak function signature respect error_arg for allow_rename = FALSE #358

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
519267e
`use_standalone("r-lib/rlang")` `purrr` and `obj-type` + update snaps…
olivroy Oct 23, 2024
05149d8
Tweak function signature to be more consistent
olivroy Oct 23, 2024
e734c6c
Use regular snapshot tests with `cnd_class`
olivroy Oct 23, 2024
a761011
use standard type checking
olivroy Oct 23, 2024
9938eb5
Respect `error_arg` with `allow_rename = FALSE`
olivroy Oct 23, 2024
972a200
Standardize on putting `cnd_class = TRUE` on the same line to make it…
olivroy Oct 23, 2024
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
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Imports:
cli (>= 3.3.0),
glue (>= 1.3.0),
lifecycle (>= 1.0.3),
rlang (>= 1.0.4),
rlang (>= 1.1.0),
vctrs (>= 0.5.2),
withr
Suggests:
Expand Down
5 changes: 1 addition & 4 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
# tidyselect (development version)

* `eval_select(allow_empty = FALSE)` gains a new argument to yield a better error
message in case of empty selection (@olivroy, #327)

* `eval_select()` and `eval_relocate()` gain a new `error_arg` argument that can be specified to throw a better error message when `allow_empty = FALSE`.
* `eval_select()` and `eval_relocate()` gain a new `error_arg` argument that can be specified to throw a better error message when `allow_empty = FALSE` or `allow_rename = FALSE` (@olivroy, #327).

* `eval_select()` and `eval_relocate()` throw a classed error message when `allow_empty = FALSE` (@olivroy, #347).

Expand Down
226 changes: 0 additions & 226 deletions R/compat-type.R
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions R/eval-relocate.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ eval_relocate <- function(expr,
allow_rename = TRUE,
allow_empty = TRUE,
allow_predicates = TRUE,
error_arg = NULL,
before_arg = "before",
after_arg = "after",
error_arg = NULL,
env = caller_env(),
error_call = caller_env()) {
check_dots_empty()
Expand All @@ -79,6 +79,7 @@ eval_relocate <- function(expr,
data <- tidyselect_data_proxy(data)

expr <- as_quosure(expr, env = env)

sel <- eval_select_impl(
x = data,
names = names(data),
Expand All @@ -88,8 +89,8 @@ eval_relocate <- function(expr,
allow_rename = allow_rename,
allow_empty = allow_empty,
allow_predicates = allow_predicates,
error_arg = error_arg,
type = "relocate",
error_arg = error_arg,
error_call = error_call
)

Expand Down
14 changes: 5 additions & 9 deletions R/eval-select.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
#' support predicates (as determined by [tidyselect_data_has_predicates()]).
#' @param error_arg Argument names for `expr`. These
#' are used in error messages. (You can use `"..."` if `expr = c(...)`).
#' For now, this is used when `allow_empty = FALSE`.
#' For now, this is used when `allow_empty = FALSE` or `allow_rename = FALSE`.
#' @inheritParams rlang::args_dots_empty
#'
#' @return A named vector of numeric locations, one for each of the
Expand Down Expand Up @@ -201,23 +201,21 @@ eval_select_impl <- function(x,
allow_rename = allow_rename,
allow_empty = allow_empty,
allow_predicates = allow_predicates,
type = type,
error_arg = error_arg,
type = type,
error_call = error_call
),
type = type
)

if (length(include) > 0) {
if (!is.character(include)) {
cli::cli_abort("{.arg include} must be a character vector.", call = error_call)
}
check_character(include, call = error_call)

missing <- setdiff(include, names)
if (length(missing) > 0) {
cli::cli_abort(c(
"{.arg include} must only include variables found in {.arg data}.",
i = "Unknown variables: {.and {missing}}"
i = "Unknown variables: {.var {missing}}"
), call = error_call)
}

Expand All @@ -228,9 +226,7 @@ eval_select_impl <- function(x,
}

if (length(exclude) > 0) {
if (!is.character(exclude)) {
cli::cli_abort("{.arg include} must be a character vector.", call = error_call)
}
check_character(exclude, call = error_call)

to_exclude <- vctrs::vec_match(intersect(exclude, names), names)
out <- out[!out %in% to_exclude]
Expand Down
21 changes: 16 additions & 5 deletions R/eval-walk.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,22 @@ ensure_named <- function(pos,
check_empty(pos, allow_empty, error_arg, call = call)

if (!allow_rename && any(names2(pos) != "")) {
cli::cli_abort(
"Can't rename variables in this context.",
class = "tidyselect:::error_disallowed_rename",
call = call
)
if (is.null(error_arg)) {
cli::cli_abort(
"Can't rename variables in this context.",
class = "tidyselect:::error_disallowed_rename",
call = call
)
} else {
cli::cli_abort(c(
"Can't rename variables in this context.",
i = "{.arg {error_arg}} can't be renamed."
),
class = "tidyselect:::error_disallowed_rename",
call = call
)
}
Comment on lines +113 to +127
Copy link
Member

Choose a reason for hiding this comment

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

In #336 I think I was hoping that we could somehow mention the expression where the rename happens in the error message.

ensure_named() is only called by vars_select_eval() and that does have the expr. So you could pass expr through to ensure_named() and use as_label(expr) on it to get something nice to include in the error message. Like

      expr <- as_label(expr)
      cli::cli_abort(c(
        "Can't rename variables during selection.",
        i = "Rename detected in {.arg {error_arg}}, with expression {.code {expr}}."
        ),
        class = "tidyselect:::error_disallowed_rename",
        call = call
      )

For something like c(all_of(x), y) if the rename happens in the all_of(x), it would still report the full c(all_of(x), y) as the place where the rename happened, but I think that is ok, it is hard to be more precise. I still think this would be a big improvement.

I think adding the argument name is nice, but maybe not quite as good as it could be on its own. For example this dplyr error from tidyverse/dplyr#6745 would have been:

#> Error in `rename_with()`:
#> ! Can't rename variables in this context.
#> i `.cols` can't be renamed.

But the user typed in mtcars %>% rename_with(toupper, all_of(v)) so .cols isn't immediately meaningful to them. But this:

#> Error in `rename_with()`:
#> ! Can't rename variables in this context.
#> i Rename detected in `.cols`, with expression `all_of(v)`.

is probably enough for them to figure out where to look next, because it refers to something in their code

WDYT @lionel- ?

Copy link
Member

Choose a reason for hiding this comment

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

I like it!

Copy link
Member

Choose a reason for hiding this comment

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

I think we could use the "In argument" pattern for consistency.

#> Error in `rename_with()`:
#> ! Can't rename variables in this tidyselect context.
#> i In argument: `.cols = all_of(v)`.


}

nms <- names(pos) <- names2(pos)
Expand Down
5 changes: 1 addition & 4 deletions R/helpers-misc.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,7 @@ everything <- function(vars = NULL) {
#' @export
#' @param offset Set it to `n` to select the nth var from the end.
last_col <- function(offset = 0L, vars = NULL) {
if (!is_integerish(offset, n = 1)) {
not <- obj_type_friendly(offset)
cli::cli_abort("{.arg offset} must be a single integer, not {not}.")
}
check_number_whole(offset)

vars <- vars %||% peek_vars(fn = "last_col")
n <- length(vars)
Expand Down
Loading
Loading