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

Support error_arg for allow_rename = FALSE as well #359

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Oct 23, 2024

Closes #336, follow-up to #350

See comments from review in #358 (comment)

R/eval-relocate.R Outdated Show resolved Hide resolved
R/eval-relocate.R Outdated Show resolved Hide resolved
R/eval-select.R Outdated Show resolved Hide resolved
R/eval-select.R Outdated Show resolved Hide resolved
R/eval-walk.R Outdated
Comment on lines 113 to 127
if (is.null(error_arg)) {
cli::cli_abort(
"Can't rename variables in this context.",
class = "tidyselect:::error_disallowed_rename",
call = call
)
} else {
cli::cli_abort(c(
"Can't rename variables in this context.",
i = "{.arg {error_arg}} can't be renamed."
),
class = "tidyselect:::error_disallowed_rename",
call = call
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe construct the message inside the if branch, but then fall through to a single call to cli_abort() so we dont duplicate the class name? (that feels like a place where we could make a typo to get out of sync). Maybe like

message <- "Can't rename variables in this context."

if (!is.null(error_arg)) {
  message <- c(message, i = cli::format_inline("{.arg {error_arg}} can't be renamed."))
}

cli::cli_abort(
  message,
  class = "tidyselect:::error_disallowed_rename",
  call = call
)

(Or maybe apply this general idea to the final error message that incorporates the expr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I caught a copy paste typo while working on this.

I applied a similar logic to another duplicated error class I created

Copy link
Member

Choose a reason for hiding this comment

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

In that case it's good practice to still interpolate message in a string like this:

cli::cli_abort(
  "{message}",
  class = "tidyselect:::error_disallowed_rename",
  call = call
)

This prevents unexpected interpolation/interpretation of components of messages, esp. if it comes from external sources (little bobby tables).

Copy link
Member

Choose a reason for hiding this comment

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

oh but here it's not as straightforward because we're adding an element to the vector... I'm just going to revert this change then.

@lionel- lionel- merged commit b16c333 into r-lib:main Oct 25, 2024
11 checks passed
@lionel-
Copy link
Member

lionel- commented Oct 25, 2024

Thanks for all these!

@lionel- lionel- mentioned this pull request Oct 25, 2024
@olivroy olivroy deleted the allow-rename-false branch October 28, 2024 14:55
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.

Can we add more context for Can't rename in this context errors?
3 participants