Quietly deprecate optional .cols and .fns cases
#6523
Conversation
With some complex changes to ensure that warnings thrown outside of any group context still bubble up correctly
| cnd <- catch_cnd(classes = "lifecycle_warning_deprecated", { | ||
| lifecycle::deprecate_warn( | ||
| when = "1.1.0", | ||
| what = I(glue("Using `{across_if_fn}()` without supplying `.cols`")), | ||
| details = "Please supply `.cols` instead.", | ||
| user_env = user_env | ||
| ) | ||
| }) | ||
|
|
||
| if (is_null(cnd)) { | ||
| # Condition wasn't signaled | ||
| return(NULL) | ||
| } | ||
|
|
||
| # Subclassed so we can skip computing group context info when the warning | ||
| # is thrown from `expand_across()` outside of any group | ||
| class(cnd) <- c("dplyr:::warning_across_missing_cols_deprecated", class(cnd)) |
There was a problem hiding this comment.
This is a bit more complicated than I'd like, but I think we have to do it.
Without this, dplyr will try and add group context to the warning, but this warning is thrown from expand_across() which is outside of any group context, and an error is thrown.
We have a system in place to handle a few other cases like this, which involves checking if the condition is one of a set of known errors/warnings to "skip" computing group context for, but that requires adding a custom class to check against.
There was a problem hiding this comment.
I guess we should add a class argument to deprecate_warn() to make this sort of things easier? Probably too late to add ... for attributes, but we could add fields = list() instead.
| cnd_data <- function(cnd, ctxt, mask_type, call) { | ||
| needs_group_context <- needs_group_context(cnd) | ||
|
|
||
| if (needs_group_context) { | ||
| group_data <- cur_group_data(mask_type) | ||
| } else { | ||
| group_data <- NULL | ||
| } |
There was a problem hiding this comment.
The dplyr_error_handler() knew how to check needs_group_context(), but the dplyr_warning_handler() didn't. This adds support to the warning handler too so the deprecation warning can pass through.
| out <- mutate(gdf, z = across()) | ||
| Condition | ||
| Warning: | ||
| There were 2 warnings in `mutate()`. |
There was a problem hiding this comment.
Only 1 warning is actually thrown when called interactively. The verbosity option is set to always warn, so we get 2 here when there are 2 groups
There was a problem hiding this comment.
I originally didn't have tests for the grouped case, but since I had to make a few internal changes to actually support throwing the warning in the grouped case, it felt important to add some tests cases too.
| #' @seealso [c_across()] for a function that returns a vector | ||
| across <- function(.cols = everything(), | ||
| .fns = NULL, | ||
| across <- function(.cols, |
There was a problem hiding this comment.
It still seems strange to me that we made across() without args work in dplyr 1.0 but we now require an argument for pick() in dplyr 1.1.
There was a problem hiding this comment.
we made across() without args work in dplyr 1.0
That was the version across() was introduced in though, I don't think we decided that it should be everything() after hearing from users that they wanted it to be that way. It was just this commit 57f2c27
We want to discourage people from doing across(, .fns = mean) to apply mean to every column in an ugly way, so I do think this is the right change to make jointly with the .fns change.
But I guess you are talking about the fact that transitioning from across() could look like this for anyone who previously used it to select everything.
across() -> pick(everything())I still don't mind this too much, I like that it is explicit and I'm not convinced it is actually that common, but if you feel strongly about it I would be willing to make pick() with no arguments select everything.
There was a problem hiding this comment.
I think materialising the current data seems like such a fundamental operation (this does not mean common) that it's strange that it requires another argument that is so much longer. I don't feel very strongly about it though.
There was a problem hiding this comment.
I think we should keep it strict for now, and then if we hear a lot of complaints we can relax it so pick() is equivalent to pick(everything()).
| cnd <- catch_cnd(classes = "lifecycle_warning_deprecated", { | ||
| lifecycle::deprecate_warn( | ||
| when = "1.1.0", | ||
| what = I(glue("Using `{across_if_fn}()` without supplying `.cols`")), | ||
| details = "Please supply `.cols` instead.", | ||
| user_env = user_env | ||
| ) | ||
| }) | ||
|
|
||
| if (is_null(cnd)) { | ||
| # Condition wasn't signaled | ||
| return(NULL) | ||
| } | ||
|
|
||
| # Subclassed so we can skip computing group context info when the warning | ||
| # is thrown from `expand_across()` outside of any group | ||
| class(cnd) <- c("dplyr:::warning_across_missing_cols_deprecated", class(cnd)) |
There was a problem hiding this comment.
I guess we should add a class argument to deprecate_warn() to make this sort of things easier? Probably too late to add ... for attributes, but we could add fields = list() instead.
| # complicated, so instead we force the global environment. This means users | ||
| # won't ever see the "deprecated feature was likely used in the {pkg}" | ||
| # message, but the warning will still fire and that is more important. | ||
| user_env <- global_env() |
There was a problem hiding this comment.
Nice pattern, this should be documented in lifecycle: r-lib/lifecycle#146
It'd be nice to restore the infrastructure to obtain the user env though.
3 participants
Follow up to #6492
.colsand.fnsare now required foracross(),c_across(),if_all(), andif_any().We recommend in the NEWS that you use
pick()instead ofacross(.fns = NULL), which was the most common case of how people relied on these defaultsThis deprecation is currently hidden, all calls to these functions that rely on the previous defaults still work with no messages or warnings. I think that is definitely the right thing to do forThe deprecation is hidden for.fns = NULL, because people would often doacross(c(x, y))to mimic whatpick()now does, and previously there was no other option. But I wonder if we could go ahead and soft deprecate the.colscase?.fns = NULL, but we now usedeprecate_warn()for missing.cols