From 0024e3763bc091939357602d2be734de6a027106 Mon Sep 17 00:00:00 2001 From: olivroy Date: Wed, 23 Oct 2024 14:59:00 -0400 Subject: [PATCH 1/6] Add support for `error_arg` for `allow_rename = FALSE` --- NEWS.md | 5 +---- R/eval-relocate.R | 5 +++-- R/eval-select.R | 4 ++-- R/eval-walk.R | 21 ++++++++++++++++----- R/utils.R | 4 ++-- man/eval_relocate.Rd | 10 +++++----- man/eval_select.Rd | 2 +- tests/testthat/test-eval-relocate.R | 16 +++++++++------- tests/testthat/test-eval-select.R | 11 ++++++----- 9 files changed, 45 insertions(+), 33 deletions(-) diff --git a/NEWS.md b/NEWS.md index 0b551a1..497fd6e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -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). diff --git a/R/eval-relocate.R b/R/eval-relocate.R index 2d1360d..7fe60da 100644 --- a/R/eval-relocate.R +++ b/R/eval-relocate.R @@ -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() @@ -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), @@ -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 ) diff --git a/R/eval-select.R b/R/eval-select.R index f57a436..2884f76 100644 --- a/R/eval-select.R +++ b/R/eval-select.R @@ -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 @@ -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, error_call = error_call ), type = type diff --git a/R/eval-walk.R b/R/eval-walk.R index 5923a83..170dee7 100644 --- a/R/eval-walk.R +++ b/R/eval-walk.R @@ -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 + ) + } + } nms <- names(pos) <- names2(pos) diff --git a/R/utils.R b/R/utils.R index c5b2895..5edaba0 100644 --- a/R/utils.R +++ b/R/utils.R @@ -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() @@ -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 ) } diff --git a/man/eval_relocate.Rd b/man/eval_relocate.Rd index 5c08dc0..c460878 100644 --- a/man/eval_relocate.Rd +++ b/man/eval_relocate.Rd @@ -15,9 +15,9 @@ eval_relocate( 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() ) @@ -60,13 +60,13 @@ use predicates (i.e. in \code{where()}). If \code{FALSE}, will error if \code{ex predicate. Will automatically be set to \code{FALSE} if \code{data} does not support predicates (as determined by \code{\link[=tidyselect_data_has_predicates]{tidyselect_data_has_predicates()}}).} -\item{error_arg}{Argument names for \code{expr}. These -are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}). -For now, this is used when \code{allow_empty = FALSE}.} - \item{before_arg, after_arg}{Argument names for \code{before} and \code{after}. These are used in error messages.} +\item{error_arg}{Argument names for \code{expr}. These +are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}). +For now, this is used when \code{allow_empty = FALSE} or \code{allow_rename = FALSE}.} + \item{env}{The environment in which to evaluate \code{expr}. Discarded if \code{expr} is a \link[rlang:enquo]{quosure}.} diff --git a/man/eval_select.Rd b/man/eval_select.Rd index 19d85b0..635f779 100644 --- a/man/eval_select.Rd +++ b/man/eval_select.Rd @@ -78,7 +78,7 @@ selection.} \item{error_arg}{Argument names for \code{expr}. These are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}). -For now, this is used when \code{allow_empty = FALSE}.} +For now, this is used when \code{allow_empty = FALSE} or \code{allow_rename = FALSE}.} } \value{ A named vector of numeric locations, one for each of the diff --git a/tests/testthat/test-eval-relocate.R b/tests/testthat/test-eval-relocate.R index 02cc6e2..861c1bb 100644 --- a/tests/testthat/test-eval-relocate.R +++ b/tests/testthat/test-eval-relocate.R @@ -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")) @@ -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) }) }) diff --git a/tests/testthat/test-eval-select.R b/tests/testthat/test-eval-select.R index 9a3721e..8d2eff2 100644 --- a/tests/testthat/test-eval-select.R +++ b/tests/testthat/test-eval-select.R @@ -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")) From 75cf6d5e49152e8b7b0362daaf6e851679706e48 Mon Sep 17 00:00:00 2001 From: olivroy Date: Wed, 23 Oct 2024 14:59:21 -0400 Subject: [PATCH 2/6] Update snapshots --- tests/testthat/_snaps/eval-relocate.md | 31 +++++++++++++------------- tests/testthat/_snaps/eval-select.md | 26 +++++++++++---------- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/tests/testthat/_snaps/eval-relocate.md b/tests/testthat/_snaps/eval-relocate.md index 5c531e7..91bb831 100644 --- a/tests/testthat/_snaps/eval-relocate.md +++ b/tests/testthat/_snaps/eval-relocate.md @@ -67,36 +67,37 @@ # can forbid rename syntax Code - (expect_error(relocate_loc(x, c(foo = b), allow_rename = FALSE))) - Output - + relocate_loc(x, c(foo = b), allow_rename = FALSE) + Condition 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 - + relocate_loc(x, c(b, foo = b), allow_rename = FALSE) + Condition + 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 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 - + 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 - + 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 - + relocate_loc(mtcars, starts_with("z"), allow_empty = FALSE) + Condition Error in `relocate_loc()`: ! Must select at least one item. diff --git a/tests/testthat/_snaps/eval-select.md b/tests/testthat/_snaps/eval-select.md index 13264cf..256361c 100644 --- a/tests/testthat/_snaps/eval-select.md +++ b/tests/testthat/_snaps/eval-select.md @@ -24,29 +24,31 @@ # can forbid rename syntax (#178) Code - (expect_error(select_loc(mtcars, c(foo = cyl), allow_rename = FALSE))) - Output - + select_loc(mtcars, c(foo = cyl), allow_rename = FALSE) + Condition 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 - + select_loc(mtcars, c(cyl, foo = cyl), allow_rename = FALSE) + Condition 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 - + select_loc(mtcars, c(cyl, foo = mpg), allow_rename = FALSE) + Condition 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 - + select_loc(mtcars, c(foo = mpg, cyl), allow_rename = FALSE) + Condition + 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 Error in `select_loc()`: ! Can't rename variables in this context. + i `x` can't be renamed. # can forbid empty selections From d61cd6480e76fef4bdbd9cf8ec255f4bf5554098 Mon Sep 17 00:00:00 2001 From: olivroy Date: Thu, 24 Oct 2024 08:16:20 -0400 Subject: [PATCH 3/6] address comments move back error_arg down --- R/eval-relocate.R | 8 ++++---- R/eval-select.R | 7 +++---- man/eval_relocate.Rd | 9 ++++----- man/eval_select.Rd | 3 +-- tests/testthat/_snaps/eval-select.md | 2 +- 5 files changed, 13 insertions(+), 16 deletions(-) diff --git a/R/eval-relocate.R b/R/eval-relocate.R index 7fe60da..ddd2232 100644 --- a/R/eval-relocate.R +++ b/R/eval-relocate.R @@ -70,8 +70,8 @@ eval_relocate <- function(expr, allow_predicates = TRUE, before_arg = "before", after_arg = "after", - error_arg = NULL, env = caller_env(), + error_arg = NULL, error_call = caller_env()) { check_dots_empty() @@ -89,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 ) @@ -125,7 +125,7 @@ eval_relocate <- function(expr, error_call = error_call, allow_predicates = allow_predicates, allow_rename = FALSE, - error_arg = before_arg + error_arg = error_arg ), arg = before_arg, error_call = error_call @@ -147,7 +147,7 @@ eval_relocate <- function(expr, error_call = error_call, allow_predicates = allow_predicates, allow_rename = FALSE, - error_arg = after_arg + error_arg = error_arg ), arg = after_arg, error_call = error_call diff --git a/R/eval-select.R b/R/eval-select.R index 2884f76..23dfeda 100644 --- a/R/eval-select.R +++ b/R/eval-select.R @@ -45,7 +45,6 @@ #' 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` or `allow_rename = FALSE`. #' @inheritParams rlang::args_dots_empty #' #' @return A named vector of numeric locations, one for each of the @@ -174,8 +173,8 @@ eval_select_impl <- function(x, allow_rename = TRUE, allow_empty = TRUE, allow_predicates = TRUE, - error_arg = NULL, type = "select", + error_arg = NULL, error_call = caller_env()) { if (!is_null(x)) { vctrs::vec_assert(x) @@ -201,8 +200,8 @@ eval_select_impl <- function(x, allow_rename = allow_rename, allow_empty = allow_empty, allow_predicates = allow_predicates, - error_arg = error_arg, type = type, + error_arg = error_arg, error_call = error_call ), type = type @@ -229,7 +228,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) + cli::cli_abort("{.arg exclude} must be a character vector.", call = error_call) } to_exclude <- vctrs::vec_match(intersect(exclude, names), names) diff --git a/man/eval_relocate.Rd b/man/eval_relocate.Rd index c460878..0a8fd44 100644 --- a/man/eval_relocate.Rd +++ b/man/eval_relocate.Rd @@ -17,8 +17,8 @@ eval_relocate( allow_predicates = TRUE, before_arg = "before", after_arg = "after", - error_arg = NULL, env = caller_env(), + error_arg = NULL, error_call = caller_env() ) } @@ -63,13 +63,12 @@ support predicates (as determined by \code{\link[=tidyselect_data_has_predicates \item{before_arg, after_arg}{Argument names for \code{before} and \code{after}. These are used in error messages.} -\item{error_arg}{Argument names for \code{expr}. These -are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}). -For now, this is used when \code{allow_empty = FALSE} or \code{allow_rename = FALSE}.} - \item{env}{The environment in which to evaluate \code{expr}. Discarded if \code{expr} is a \link[rlang:enquo]{quosure}.} +\item{error_arg}{Argument names for \code{expr}. These +are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}).} + \item{error_call}{The execution environment of a currently running function, e.g. \code{caller_env()}. The function will be mentioned in error messages as the source of the error. See the diff --git a/man/eval_select.Rd b/man/eval_select.Rd index 635f779..b33a7ed 100644 --- a/man/eval_select.Rd +++ b/man/eval_select.Rd @@ -77,8 +77,7 @@ in an empty selection. If \code{FALSE}, will error if \code{expr} yields an empt selection.} \item{error_arg}{Argument names for \code{expr}. These -are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}). -For now, this is used when \code{allow_empty = FALSE} or \code{allow_rename = FALSE}.} +are used in error messages. (You can use \code{"..."} if \code{expr = c(...)}).} } \value{ A named vector of numeric locations, one for each of the diff --git a/tests/testthat/_snaps/eval-select.md b/tests/testthat/_snaps/eval-select.md index 5989f32..c401f56 100644 --- a/tests/testthat/_snaps/eval-select.md +++ b/tests/testthat/_snaps/eval-select.md @@ -16,7 +16,7 @@ select_loc(x, "a", exclude = 1) Condition Error in `select_loc()`: - ! `include` must be a character vector. + ! `exclude` must be a character vector. # can forbid rename syntax (#178) From a759189be18be931625fcbb13e2f5db8ab887f9d Mon Sep 17 00:00:00 2001 From: olivroy Date: Thu, 24 Oct 2024 08:27:07 -0400 Subject: [PATCH 4/6] those are actually not used, removing --- R/eval-relocate.R | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/R/eval-relocate.R b/R/eval-relocate.R index ddd2232..91b199a 100644 --- a/R/eval-relocate.R +++ b/R/eval-relocate.R @@ -124,8 +124,7 @@ eval_relocate <- function(expr, env = env, error_call = error_call, allow_predicates = allow_predicates, - allow_rename = FALSE, - error_arg = error_arg + allow_rename = FALSE ), arg = before_arg, error_call = error_call @@ -146,8 +145,7 @@ eval_relocate <- function(expr, env = env, error_call = error_call, allow_predicates = allow_predicates, - allow_rename = FALSE, - error_arg = error_arg + allow_rename = FALSE ), arg = after_arg, error_call = error_call From 2db88460d628410e00eb3f837b423be289aac2b2 Mon Sep 17 00:00:00 2001 From: olivroy Date: Thu, 24 Oct 2024 16:17:44 -0400 Subject: [PATCH 5/6] avoid code duplicstion --- R/eval-walk.R | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/R/eval-walk.R b/R/eval-walk.R index 42ff6e6..b2b7551 100644 --- a/R/eval-walk.R +++ b/R/eval-walk.R @@ -108,24 +108,19 @@ ensure_named <- function(pos, error_arg = NULL, call = caller_env()) { check_empty(pos, allow_empty, error_arg, call = call) + if (!allow_rename && any(names2(pos) != "")) { - 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 - ) + msg <- "Can't rename variables in this context." + # Add more context if error_arg is supplied. + if (!is.null(error_arg)) { + msg <- c(msg, "i" = "{.arg {error_arg}} can't be renamed.") } - + cli::cli_abort( + msg, + class = "tidyselect:::error_disallowed_rename", + call = call + ) } nms <- names(pos) <- names2(pos) @@ -143,18 +138,15 @@ ensure_named <- function(pos, check_empty <- function(x, allow_empty = TRUE, error_arg = NULL, call = caller_env()) { if (!allow_empty && length(x) == 0) { if (is.null(error_arg)) { - cli::cli_abort( - "Must select at least one item.", - call = call, - class = "tidyselect_error_empty_selection" - ) + msg <- "Must select at least one item." } else { - cli::cli_abort( - "{.arg {error_arg}} must select at least one column.", - call = call, - class = "tidyselect_error_empty_selection" - ) + msg <- "{.arg {error_arg}} must select at least one column." } + cli::cli_abort( + msg, + call = call, + class = "tidyselect_error_empty_selection" + ) } } From 1f7dfa49c5cd10f20d62dcbeb1a0deab058876d3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 25 Oct 2024 10:23:56 +0200 Subject: [PATCH 6/6] Always pass a string to `cli_abort()` --- R/eval-walk.R | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/R/eval-walk.R b/R/eval-walk.R index b2b7551..ee7e0be 100644 --- a/R/eval-walk.R +++ b/R/eval-walk.R @@ -108,19 +108,24 @@ ensure_named <- function(pos, error_arg = NULL, call = caller_env()) { check_empty(pos, allow_empty, error_arg, call = call) - if (!allow_rename && any(names2(pos) != "")) { - msg <- "Can't rename variables in this context." - # Add more context if error_arg is supplied. - if (!is.null(error_arg)) { - msg <- c(msg, "i" = "{.arg {error_arg}} can't be renamed.") + 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 + ) } - cli::cli_abort( - msg, - class = "tidyselect:::error_disallowed_rename", - call = call - ) } nms <- names(pos) <- names2(pos) @@ -140,10 +145,10 @@ check_empty <- function(x, allow_empty = TRUE, error_arg = NULL, call = caller_e if (is.null(error_arg)) { msg <- "Must select at least one item." } else { - msg <- "{.arg {error_arg}} must select at least one column." + msg <- cli::format_inline("{.arg {error_arg}} must select at least one column.") } cli::cli_abort( - msg, + "{msg}", call = call, class = "tidyselect_error_empty_selection" )