-
Notifications
You must be signed in to change notification settings - Fork 39
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
Conversation
R/eval-walk.R
Outdated
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 | ||
) |
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.
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
)
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.
Good idea! I caught a copy paste typo while working on this.
I applied a similar logic to another duplicated error class I created
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.
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).
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.
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.
7f3e858
to
501a27c
Compare
501a27c
to
1f7dfa4
Compare
Thanks for all these! |
Closes #336, follow-up to #350
See comments from review in #358 (comment)