Improve error message + tweak function signature respect error_arg for allow_rename = FALSE

[olivroy]

I split the PR up

• Use recent standalone files Update standalone files #360
• Tweak function signature (from Throw classed error if allow_empty = FALSE + add more context to error message by adding error_arg to eval_select() #350 ) and support error_arg for allow_rename = FALSE Support error_arg for allow_rename = FALSE as well #359
• Use standard snapshot tests with cnd_class = TRUE when appropriate. Avoid nested expect_error() in snapshot tests #361

No rush, but I wondered if you had a date in mind for a cran release?

[olivroy]

With 9938eb5, I may have a solution for #327, #336.

I will test it with dplyr / tidyr to see if it works well.

[DavisVaughan]

Would it be possible to make this into multiple much smaller PRs that do exactly 1 thing?
https://code-review.tidyverse.org/author/focused.html

It is much easier for us to confidently review a PR when it does 1 thing. In particular this is 3-4 PRs:

• The i = "{.arg {error_arg}} can't be renamed." error bullet
• Updating to use_standalone() helpers, and updating type checker usage
• Modernizing snapshot tests
• Changing a function signature

These each deserve their own PR where we can have detailed but focused conversation about each idea! I do love the enthusiasm though, and we really appreciate these contributions, they look great!

[DavisVaughan]

In #336 I think I was hoping that we could somehow mention the expression where the rename happens in the error message.

ensure_named() is only called by vars_select_eval() and that does have the expr. So you could pass expr through to ensure_named() and use as_label(expr) on it to get something nice to include in the error message. Like

      expr <- as_label(expr)
      cli::cli_abort(c(
        "Can't rename variables during selection.",
        i = "Rename detected in {.arg {error_arg}}, with expression {.code {expr}}."
        ),
        class = "tidyselect:::error_disallowed_rename",
        call = call
      )

For something like c(all_of(x), y) if the rename happens in the all_of(x), it would still report the full c(all_of(x), y) as the place where the rename happened, but I think that is ok, it is hard to be more precise. I still think this would be a big improvement.

---

I think adding the argument name is nice, but maybe not quite as good as it could be on its own. For example this dplyr error from tidyverse/dplyr#6745 would have been:

#> Error in `rename_with()`:
#> ! Can't rename variables in this context.
#> i `.cols` can't be renamed.

But the user typed in mtcars %>% rename_with(toupper, all_of(v)) so .cols isn't immediately meaningful to them. But this:

#> Error in `rename_with()`:
#> ! Can't rename variables in this context.
#> i Rename detected in `.cols`, with expression `all_of(v)`.

is probably enough for them to figure out where to look next, because it refers to something in their code

WDYT @lionel- ?

[lionel-]

I like it!

[lionel-]

I think we could use the "In argument" pattern for consistency.

#> Error in `rename_with()`:
#> ! Can't rename variables in this tidyselect context.
#> i In argument: `.cols = all_of(v)`.

[lionel-]

Closing this since it was split into new PRs.