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.

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

Throw classed error if allow_empty = FALSE + add more context to error message by adding error_arg to eval_select() #350

Merged
merged 9 commits into from
Oct 23, 2024

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Apr 25, 2024

Closes #347

basically implement what was raised in #282 (comment)

Addresses part of #327

I have to say this fix is a bit naive and narrow and lacks a more general view.

pkgload::load_all() 
#> ℹ Loading tidyselect
# give a better error message if you don't want empty selection
 # if supplying `error_arg`
 select_not_empty <- function(x, cols) {
   eval_select(expr = enquo(cols), data = x, allow_empty = FALSE, error_arg = "cols")
 }
 try(select_not_empty(mtcars, cols = starts_with("vs2")))
#> Error in select_not_empty(mtcars, cols = starts_with("vs2")) : 
#>   `cols` must select at least one column.

Created on 2024-04-25 with reprex v2.1.0

Would it make sense to recognize it in eval_relocate()?

A version I had locally had error_arg = c(before_arg, after_arg), but couldn't tell if it was useful.

This implementation doesn't change the default, which mean it is opt-in.

Reason behind.

I was working on gt and discovered allow_empty and I thought I'd use it to throw messages, (without having to rethrow it again)

But I found that the error message Can't select empty items was not informative at all, so it is required to rethrow it or make tweaks in tidyselect.

So I thought it would be great if tidyselect had a mechanism to do this, and discovered it was proposed last time tidyselect was updated, but was never implemented.

@olivroy olivroy changed the title Error arg Throw classed error if allow_empty = FALSE fails + add more context by adding error_arg to eval_select() Apr 25, 2024
@olivroy olivroy changed the title Throw classed error if allow_empty = FALSE fails + add more context by adding error_arg to eval_select() Throw classed error if allow_empty = FALSE + add more context to error message by adding error_arg to eval_select() Apr 26, 2024
Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look! Just have some comments, please let me know if you need me to finish this up.

NAMESPACE Show resolved Hide resolved
R/eval-select.R Outdated
@@ -43,6 +43,8 @@
#' use predicates (i.e. in `where()`). If `FALSE`, will error if `expr` uses a
#' predicate. Will automatically be set to `FALSE` if `data` does not
#' support predicates (as determined by [tidyselect_data_has_predicates()]).
#' @param error_arg Argument name to include in error message if `allow_empty = FALSE`.
Copy link
Member

Choose a reason for hiding this comment

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

This should mention errors in general, as it might be used more generally in the future.

Should also reference it's for expr, and that it can be set to "..." if called with expr = c(...).

Copy link
Member

Choose a reason for hiding this comment

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

Since you use .or, mention this can be a character vector of argument names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally, it should never resolve to being multiple arguments. Since before and after being both supplied should result in another error message.. (can't supply after and before

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah, makes sense!

R/eval-select.R Outdated Show resolved Hide resolved
R/eval-relocate.R Outdated Show resolved Hide resolved
R/eval-walk.R Outdated Show resolved Hide resolved
R/eval-walk.R Outdated Show resolved Hide resolved
@@ -105,6 +105,14 @@ test_that("can forbid empty selections", {
})
})

test_that("can forbid empty selections with informative error", {
Copy link
Member

Choose a reason for hiding this comment

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

Need relocation tests as well?

@olivroy
Copy link
Contributor Author

olivroy commented Oct 22, 2024

Added the argument to eval_relocate() as well. I realized that before_arg and after_arg are not involved in all this. before and after will error elsewhere if they end up being empty.

Edit: See also https://github.com/tidyverse/tidyr/pull/1549/files where I attempted to use this! I think a few packages will benefit from this once released to CRAN.

@lionel- lionel- merged commit e4190a9 into r-lib:main Oct 23, 2024
13 checks passed
@lionel-
Copy link
Member

lionel- commented Oct 23, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Error on empty selection allow_empty = FALSE has informative class
2 participants