-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Implement case_match()
and vec_case_match()
#6328
Implement case_match()
and vec_case_match()
#6328
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
9058e02
to
9db0073
Compare
case_match()
and vec_case_match()
case_switch()
and vec_case_switch()
case_switch()
and vec_case_switch()
case_match()
and vec_case_match()
case_match()
and vec_case_match()
case_match()
and vec_case_match()
R/case-match.R
Outdated
#' # `.default` is allowed to be vectorized, and you can supply a `.ptype` to | ||
#' # force a particular output type. Combining these features together allows | ||
#' # you to create a type stable "replace match" helper. | ||
#' replace_match <- function(.x, ...) { |
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.
Are you sure we don't want to provide this function in dplyr?
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.
I haven't been able to decide if we should or not because I think it would imply we also need replace_when()
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.
Having replace_when()
and replace_match()
would be nice alternatives to replace()
though, especially it we don't do mutate(.when = )
right now
R/case_when.R
Outdated
@@ -200,30 +207,23 @@ case_when_formula_evaluate <- function(args, | |||
|
|||
for (i in seq_len(n_args)) { | |||
pair <- quos_pairs[[i]] | |||
conditions[[i]] <- eval_tidy(pair$lhs, env = default_env) | |||
values[[i]] <- eval_tidy(pair$rhs, env = default_env) | |||
lhs[[i]] <- eval_tidy(pair$lhs, env = default_env) |
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.
Should we use error wrapping here?
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.
Do you have an example of why it would be useful? Since this is just evaluating the LHS/RHS of the formula, I feel like the only error we could hit would be:
dplyr::case_when(foo ~ 1)
#> Error in eval_tidy(pair$lhs, env = default_env): object 'foo' not found
But I'm not sure we can improve on that anyways
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.
We could say which case caused the problem,
dplyr::case_match(letters, "z" ~ stop("!"))
#' Error in case_match():
#' Failed to compute right hand side of match 1
#' Caused by error:
#' !
But maybe that's not worth it?
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.
I've updated to throw a chained error like this
2bd4ec0
to
9dda617
Compare
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.
Need check/better error message for NULL
lhs and rhs:
case_match(letters, "x" ~ NULL)
case_match(letters, NULL ~ "x")
Otherwise, looks great!
Do we want to superseded recode()
in this PR or a separate one?
R/case_when.R
Outdated
@@ -200,30 +207,23 @@ case_when_formula_evaluate <- function(args, | |||
|
|||
for (i in seq_len(n_args)) { | |||
pair <- quos_pairs[[i]] | |||
conditions[[i]] <- eval_tidy(pair$lhs, env = default_env) | |||
values[[i]] <- eval_tidy(pair$rhs, env = default_env) | |||
lhs[[i]] <- eval_tidy(pair$lhs, env = default_env) |
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.
We could say which case caused the problem,
dplyr::case_match(letters, "z" ~ stop("!"))
#' Error in case_match():
#' Failed to compute right hand side of match 1
#' Caused by error:
#' !
But maybe that's not worth it?
I'll leave that for another PR, I want to get this one in |
After thinking about this more, I think this more accurately captures the intention here, and is a more applicable name in other scenarios: - `vec_case_match(needles, haystacks)` makes more sense - `fct_case_match(new_lvl = haystack)` would make more sense since the LHS here is the resulting value, not the thing you switch on - I seem to use "match" very frequently in the docs and the test descriptions, making me think that is the better name
a4c1039
to
46ee6ba
Compare
case_match()
is a variant ofcase_when()
that takes a primary input,.x
, and then a series of formulas where the LHSs of each formula are values to match against.x
rather than logical vectors. The LHSs get turned into logical conditions byvec_in()
, and then the results are passed on tovec_case_when()
.It technically closes tidyverse/funs#60
This would function as a direct successor to
recode()
, which is already questioning and has an awkward interface for anything except character vectors (and even there it can be odd).I still think a
replace_match()
would be useful here, like:replace_match()
could also be used instead of a match-like version ofna_if()
In forcats, we could have
fct_case_match()
as a successor torecode_factor()
, but its interface would probably be the other way around, like: