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

rename_with + all_of fails with named character vector for .cols #6745

Closed
LukasWallrich opened this issue Feb 19, 2023 · 4 comments
Closed

Comments

@LukasWallrich
Copy link

I was trying to rename variables quite deep in a function and ran into an odd error message for some input: Can't rename variables in this context. Apparently, that happens when the input to all_of is named ... I would expect that to make no difference here; if it is to be discouraged for some reason then a clearer error message would be very helpful.

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
v <- c(mp2 = "mpg")
mtcars %>% rename_with(toupper, all_of(v))
#> Error in `rename_with()`:
#> ! Can't rename variables in this context.

#> Backtrace:
#>      ▆
#>   1. ├─mtcars %>% rename_with(toupper, all_of(v))
#>   2. ├─dplyr::rename_with(., toupper, all_of(v))
#>   3. └─dplyr:::rename_with.data.frame(., toupper, all_of(v))
#>   4.   └─tidyselect::eval_select(enquo(.cols), .data, allow_rename = FALSE)
#>   5.     └─tidyselect:::eval_select_impl(...)
#>   6.       ├─tidyselect:::with_subscript_errors(...)
#>   7.       │ └─rlang::try_fetch(...)
#>   8.       │   └─base::withCallingHandlers(...)
#>   9.       └─tidyselect:::vars_select_eval(...)
#>  10.         └─tidyselect:::ensure_named(...)
#>  11.           └─cli::cli_abort(...)
#>  12.             └─rlang::abort(...)

Created on 2023-02-19 with reprex v2.0.2

@DavisVaughan
Copy link
Member

When renaming is allowed in combination with tidyselection, passing a named input to all_of() is one of the ways to perform this renaming, like in select()

library(dplyr, warn.conflicts = FALSE)
mtcars <- as_tibble(mtcars)
v <- c(mp2 = "mpg")
select(mtcars, all_of(v))
#> # A tibble: 32 × 1
#>      mp2
#>    <dbl>
#>  1  21  
#>  2  21  
#>  3  22.8
#>  4  21.4
#>  5  18.7
#>  6  18.1
#>  7  14.3
#>  8  24.4
#>  9  22.8
#> 10  19.2
#> # … with 22 more rows

But rename_with() uses .fn to handle how the column names are renamed, and .cols is used purely for selection.

When tidy selection is used purely for selection, like it is here, we've started tightening up our checks to ensure that the user isn't trying to also do renaming in the tidy selection. At best this would just slip by silently, but at worst it can confuse the user if they thought renaming should occur, and it can change the internal result of tidyselect::eval_select(), which has broken our internal code in the past.

I agree that this probably isn't the best error, we can probably add some context to it like we do with this:

library(tidyselect)
library(rlang)

eval_select(expr(all_of(x)), mtcars)
#> Error:
#> ! Problem while evaluating `all_of(x)`.
#> Caused by error in `as_indices_impl()`:
#> ! object 'x' not found

I'm imagining something like:

#> Error in `rename_with()`:
#> ! Problem while evaluating `all_of(x)`.
#> ! Can't rename variables in this context.

@DavisVaughan
Copy link
Member

Closing in favor of r-lib/tidyselect#336

@LukasWallrich
Copy link
Author

Thanks @DavisVaughan! Could the renaming be documented? I ran into a related issue again, and could not find a hint in the documentation of all_of? I would have expected the following two calls to do the same (i.e. to ignore the name) ... based on what you said, I understand what happened and it's great that it can rename at the same time, but currently rather surprising (happy to open an issue in rlang, but maybe this has already been discussed) ...

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
sel <- c("mpg", "cyl")
mtcars %>% mutate(across(all_of(sel), ~.x * -1)) %>% summarise(mean(mpg), mean(cyl))
#>   mean(mpg) mean(cyl)
#> 1 -20.09062   -6.1875
sel2 <- c("mpg", my = "cyl")
mtcars %>% mutate(across(all_of(sel2), ~.x * -1)) %>% summarise(mean(mpg), mean(cyl))
#>   mean(mpg) mean(cyl)
#> 1 -20.09062    6.1875

Created on 2023-02-22 with reprex v2.0.2

@DavisVaughan
Copy link
Member

There is an existing request to add some examples of named all_of()/any_of() here r-lib/tidyselect#333

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

No branches or pull requests

2 participants