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

Delay cli formatting to print-time #1273

Merged
merged 16 commits into from
Sep 13, 2021
Merged

Delay cli formatting to print-time #1273

merged 16 commits into from
Sep 13, 2021

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Sep 9, 2021

With the new displays of (1) error calls in the error prefix and (2) chained errors, eager formatting with cli can produce strange looking width-wrapping such as:

  • First line width-wrapped too early or too late if the starting column is not as expected. This would be the case with error prefixes like "Error in `foo()`: " or with chained errors

  • Chained errors which are displayed at column zero + indent of 2. IDE width-wrapping would start the newline at column 0 instead of 2.

This PR delays the formatting of bullets to print-time so that we have enough information about how to format the message nicely in context.

Formatting with cli is optional and opt-in because there is no backward compatible way of formatting arbitrary messages. That's because formatting is not idempotent. So only conditions that have a use_cli_format field set to TRUE will be formatted using cli (or the rlang fallback if not installed).

To make this easy, local_use_cli() can be called at top-level in a package to save the flag in the namespace and instruct errors thrown with abort() from inside that namespace to add the use_cli_format field. local_use_cli() can also be used within running functions or eval() environments, in which case it is a simple wrapper around local_bindings().

In future versions of cli, cli_abort() will use local_use_cli() to instruct abort() to call cli back at print-time to format. Also, the inline-formatting feature implemented in #1176 has been folded in local_use_cli(). It is disabled by default, use local_use_cli(inline = TRUE) to allow inline cli formatting and interpolation like abort("{.arg {arg}} must be a thing.").

Because conditionMessage() must return a formatted the message, which is incompatible with print-time formatting, it is now undefined behaviour to implement a method for this generic for subclasses of rlang errors that have use_cli_format set. Instead, cnd_header() (and variants) should be implemented. I have added and cross-referenced a documentation topic rlang_error to go over this.

@lionel-
Copy link
Member Author

lionel- commented Sep 9, 2021

The last commit follows up on our discussion: When a character vector is passed to abort() and local_use_cli() is on, the bullets are now stored in body. This creates a mapping between the message = vector UI of abort() and the cnd_header() / cnd_body() of rlang errors.

cnd_header(cnd),
cnd_body(cnd),
cnd_footer(cnd)
)
}

cnd_formatter <- function(cnd) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the callback to cli happens for conditions that have use_cli_format set. The indent parameter is used in the display of chained errors.

TODO: Inform cli about the starting column which may vary depending on the context. Needs r-lib/cli#345.

Copy link
Member

@gaborcsardi gaborcsardi left a comment

Choose a reason for hiding this comment

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

Which packages do you think will call local_use_cli(), in addition to cli::cli_abort() itself? Maybe we could get away without an exported function for now?

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. I didn't read the code closely.

@lionel-
Copy link
Member Author

lionel- commented Sep 13, 2021

Which packages do you think will call local_use_cli(), in addition to cli::cli_abort() itself? Maybe we could get away without an exported function for now?

Maybe we'll use abort() instead of cli_abort() to turn off inline formatting with structural formatting still enabled? It seems worthwhile to experiment with call-cli-from-rlang vs call-rlang-from-cli for a bit. This helper is marked as experimental so we can deprecate it if needed.

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.

3 participants