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

Support error_arg for allow_rename = FALSE as well #359

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Support error_arg for allow_rename = FALSE as well #359

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0024e37
Add support for `error_arg` for `allow_rename = FALSE`
olivroy Oct 23, 2024
75cf6d5
Update snapshots
olivroy Oct 23, 2024
d3ed1fd
merge
olivroy Oct 24, 2024
d61cd64
address comments move back error_arg down
olivroy Oct 24, 2024
a759189
those are actually not used, removing
olivroy Oct 24, 2024
2db8846
avoid code duplicstion
olivroy Oct 24, 2024
1f7dfa4
Always pass a string to `cli_abort()`
lionel- Oct 25, 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
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
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,
olivroy marked this conversation as resolved.
Show resolved Hide resolved
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,
olivroy marked this conversation as resolved.
Show resolved Hide resolved
type = "relocate",
error_arg = error_arg,
error_call = error_call
)

Expand Down
4 changes: 2 additions & 2 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`.
olivroy marked this conversation as resolved.
Show resolved Hide resolved
#' @inheritParams rlang::args_dots_empty
#'
#' @return A named vector of numeric locations, one for each of the
Expand Down Expand Up @@ -201,8 +201,8 @@ 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,
olivroy marked this conversation as resolved.
Show resolved Hide resolved
error_call = error_call
),
type = type
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
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe construct the message inside the if branch, but then fall through to a single call to cli_abort() so we dont duplicate the class name? (that feels like a place where we could make a typo to get out of sync). Maybe like

message <- "Can't rename variables in this context."

if (!is.null(error_arg)) {
  message <- c(message, i = cli::format_inline("{.arg {error_arg}} can't be renamed."))
}

cli::cli_abort(
  message,
  class = "tidyselect:::error_disallowed_rename",
  call = call
)

(Or maybe apply this general idea to the final error message that incorporates the expr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I caught a copy paste typo while working on this.

I applied a similar logic to another duplicated error class I created

Copy link
Member

Choose a reason for hiding this comment

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

In that case it's good practice to still interpolate message in a string like this:

cli::cli_abort(
  "{message}",
  class = "tidyselect:::error_disallowed_rename",
  call = call
)

This prevents unexpected interpolation/interpretation of components of messages, esp. if it comes from external sources (little bobby tables).

Copy link
Member

Choose a reason for hiding this comment

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

oh but here it's not as straightforward because we're adding an element to the vector... I'm just going to revert this change then.

}

}

nms <- names(pos) <- names2(pos)
Expand Down
4 changes: 2 additions & 2 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ relocate_loc <- function(x,
name_spec = NULL,
allow_rename = TRUE,
allow_empty = TRUE,
error_arg = NULL,
before_arg = "before",
after_arg = "after",
error_arg = NULL,
error_call = current_env()) {
check_dots_empty()

Expand All @@ -68,9 +68,9 @@ relocate_loc <- function(x,
name_spec = name_spec,
allow_rename = allow_rename,
allow_empty = allow_empty,
error_arg = error_arg,
before_arg = before_arg,
after_arg = after_arg,
error_arg = error_arg,
error_call = error_call
)
}
Expand Down
10 changes: 5 additions & 5 deletions man/eval_relocate.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion man/eval_select.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 16 additions & 15 deletions tests/testthat/_snaps/eval-relocate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,36 +67,37 @@
# can forbid rename syntax

Code
(expect_error(relocate_loc(x, c(foo = b), allow_rename = FALSE)))
Output
<error/tidyselect:::error_disallowed_rename>
relocate_loc(x, c(foo = b), allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `relocate_loc()`:
! Can't rename variables in this context.
Code
(expect_error(relocate_loc(x, c(b, foo = b), allow_rename = FALSE)))
Output
<error/tidyselect:::error_disallowed_rename>
relocate_loc(x, c(b, foo = b), allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `relocate_loc()`:
! Can't rename variables in this context.
Code
relocate_loc(x, c(b, foo = b), allow_rename = FALSE, error_arg = "...")
Condition <tidyselect:::error_disallowed_rename>
Error in `relocate_loc()`:
! Can't rename variables in this context.
i `...` can't be renamed.

# can forbid empty selections

Code
(expect_error(relocate_loc(x, allow_empty = FALSE, error_arg = "...")))
Output
<error/tidyselect_error_empty_selection>
relocate_loc(x, allow_empty = FALSE, error_arg = "...")
Condition
Error in `relocate_loc()`:
! `...` must select at least one column.
Code
(expect_error(relocate_loc(mtcars, integer(), allow_empty = FALSE)))
Output
<error/tidyselect_error_empty_selection>
relocate_loc(mtcars, integer(), allow_empty = FALSE)
Condition
Error in `relocate_loc()`:
! Must select at least one item.
Code
(expect_error(relocate_loc(mtcars, starts_with("z"), allow_empty = FALSE)))
Output
<error/tidyselect_error_empty_selection>
relocate_loc(mtcars, starts_with("z"), allow_empty = FALSE)
Condition
Error in `relocate_loc()`:
! Must select at least one item.

Expand Down
26 changes: 14 additions & 12 deletions tests/testthat/_snaps/eval-select.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,29 +24,31 @@
# can forbid rename syntax (#178)

Code
(expect_error(select_loc(mtcars, c(foo = cyl), allow_rename = FALSE)))
Output
<error/tidyselect:::error_disallowed_rename>
select_loc(mtcars, c(foo = cyl), allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `select_loc()`:
! Can't rename variables in this context.
Code
(expect_error(select_loc(mtcars, c(cyl, foo = cyl), allow_rename = FALSE)))
Output
<error/tidyselect:::error_disallowed_rename>
select_loc(mtcars, c(cyl, foo = cyl), allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `select_loc()`:
! Can't rename variables in this context.
Code
(expect_error(select_loc(mtcars, c(cyl, foo = mpg), allow_rename = FALSE)))
Output
<error/tidyselect:::error_disallowed_rename>
select_loc(mtcars, c(cyl, foo = mpg), allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `select_loc()`:
! Can't rename variables in this context.
Code
(expect_error(select_loc(mtcars, c(foo = mpg, cyl), allow_rename = FALSE)))
Output
<error/tidyselect:::error_disallowed_rename>
select_loc(mtcars, c(foo = mpg, cyl), allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `select_loc()`:
! Can't rename variables in this context.
Code
select_loc(mtcars, c(foo = mpg, cyl), error_arg = "x", allow_rename = FALSE)
Condition <tidyselect:::error_disallowed_rename>
Error in `select_loc()`:
! Can't rename variables in this context.
i `x` can't be renamed.

# can forbid empty selections

Expand Down
16 changes: 9 additions & 7 deletions tests/testthat/test-eval-relocate.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,10 @@ test_that("accepts name spec", {
test_that("can forbid rename syntax", {
x <- c(a = 1, b = 2, c = 3)

expect_snapshot({
(expect_error(relocate_loc(x, c(foo = b), allow_rename = FALSE)))
(expect_error(relocate_loc(x, c(b, foo = b), allow_rename = FALSE)))
expect_snapshot(error = TRUE, cnd_class = TRUE, {
relocate_loc(x, c(foo = b), allow_rename = FALSE)
relocate_loc(x, c(b, foo = b), allow_rename = FALSE)
relocate_loc(x, c(b, foo = b), allow_rename = FALSE, error_arg = "...")
})

expect_named(relocate_loc(x, c(c, b), allow_rename = FALSE), c("c", "b", "a"))
Expand All @@ -176,10 +177,11 @@ test_that("can forbid rename syntax", {
test_that("can forbid empty selections", {
x <- c(a = 1, b = 2, c = 3)

expect_snapshot({
(expect_error(relocate_loc(x, allow_empty = FALSE, error_arg = "...")))
(expect_error(relocate_loc(mtcars, integer(), allow_empty = FALSE)))
(expect_error(relocate_loc(mtcars, starts_with("z"), allow_empty = FALSE)))
expect_snapshot(error = TRUE, {
relocate_loc(x, allow_empty = FALSE, error_arg = "...")

relocate_loc(mtcars, integer(), allow_empty = FALSE)
relocate_loc(mtcars, starts_with("z"), allow_empty = FALSE)
})
})

Expand Down
11 changes: 6 additions & 5 deletions tests/testthat/test-eval-select.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,12 @@ test_that("result is named even with constant inputs (#173)", {
})

test_that("can forbid rename syntax (#178)", {
expect_snapshot({
(expect_error(select_loc(mtcars, c(foo = cyl), allow_rename = FALSE)))
(expect_error(select_loc(mtcars, c(cyl, foo = cyl), allow_rename = FALSE)))
(expect_error(select_loc(mtcars, c(cyl, foo = mpg), allow_rename = FALSE)))
(expect_error(select_loc(mtcars, c(foo = mpg, cyl), allow_rename = FALSE)))
expect_snapshot(error = TRUE, cnd_class = TRUE, {
select_loc(mtcars, c(foo = cyl), allow_rename = FALSE)
select_loc(mtcars, c(cyl, foo = cyl), allow_rename = FALSE)
select_loc(mtcars, c(cyl, foo = mpg), allow_rename = FALSE)
select_loc(mtcars, c(foo = mpg, cyl), allow_rename = FALSE)
select_loc(mtcars, c(foo = mpg, cyl), error_arg = "x", allow_rename = FALSE)
})

expect_named(select_loc(mtcars, starts_with("c") | all_of("am"), allow_rename = FALSE), c("cyl", "carb", "am"))
Expand Down
Loading