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

Format condition messages with cli #1176

Merged
merged 27 commits into from
May 12, 2021
Merged

Format condition messages with cli #1176

merged 27 commits into from
May 12, 2021

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Apr 23, 2021

Edit: Switched to a boolean flag. The "try" mode is hidden. Can now use I() to opt out of cli formatting.

  1. Take glue_env argument in abort(), warn(), and inform(). At throw timeglue_env is looked up for a .rlang_use_cli_format binding (the lookup stops at the topenv()). This flag can be set to:

    • "always" to use cli to format the message. If not available this is an internal error (caller should have added cli to Imports).
    • "try" to use cli if it is available, otherwise use the fallback formatting. This is useful to get cli width wrapping and ensure consistent formatting with user themes. The fallback doesn't perform any interpolation so if this is needed, the flag must be set to "always".
    • "never" so that the fallback formatting is used. This is the default.
  2. format_message() and format_error_message() are exported to format messages lazily from conditionMessage() or cnd_message() methods. These helpers also take an env argument and implement the cli flag logic described above. They are called from abort() and variants and can be called from condition-message methods if the error message is generated lazily.

Pros:

  • No duplication of condition API between rlang and cli

  • Behaviour is deterministic by default.

  • Can easily use cli without depending on it by setting the flag to "try". This is the mode used by rlang in this PR, which means that all rlang messages now benefit from width wrapping.

Cons:

  • Need to remember to add cli to Imports if the flag is set to "always". Could be automatised with usethis.

  • In the "try" mode, the messages will slightly vary depending on the platform because wrapping will not be applied. Luckily this does not affect unit tests since testthat depends on cli.

Closes #1090.

@lionel- lionel- force-pushed the cli-errors branch 2 times, most recently from c275af6 to 455fba7 Compare April 28, 2021 10:31
@lionel-
Copy link
Member Author

lionel- commented Apr 28, 2021

@jennybc Now uses a boolean flag. It's possible to opt out of cli formatting by supplying a message escaped with I().

@jennybc
Copy link
Member

jennybc commented Apr 29, 2021

Sort of off-topic, but still a legit part of my real-world road testing of this:

I see these warnings (a lot, i.e. every time I load_all(), test(), etc.):

Warning messages:
1: replacing previous import ‘ellipsis::check_dots_unnamed’ by ‘rlang::check_dots_unnamed’ when loading ‘tibble’ 
2: replacing previous import ‘ellipsis::check_dots_used’ by ‘rlang::check_dots_used’ when loading ‘tibble’ 
3: replacing previous import ‘ellipsis::check_dots_empty’ by ‘rlang::check_dots_empty’ when loading ‘tibble’

which is a separate matter (df2d0f3) and presumably tibble needs to respond. cc @krlmlr.

@jennybc
Copy link
Member

jennybc commented Apr 29, 2021

Observation:

The method of turning cli message formatting "on" presents a bit of a challenge for interactive development. Even after load_all(), if I just call abort() with one of my newly-inline-styled messages, to see how it will look, I don't get cli formatting. This encouraged me to create a trivial wrapper:

drive_abort <- function(...) {
  abort(...)
}

just so that .rlang_use_cli_format is found to be defined and TRUE in the namespace.

But with the above wrapper, interpolation doesn't work. So then you define it like:

drive_abort <- function(..., .glue_env = caller_env()) {
  abort(..., .glue_env = .glue_env)
}

But now you're back to not getting cli formatting in interactive work.

My understanding is that package-specific abort() wrappers are something we hope to discourage. Or at least, they won't feel necessary for many packages.

I'm not sure what the right solution is re: making this opt-in, at the package-level, but also making interactive development easier. One hack-y solution is to simply define .rlang_use_cli_format <- TRUE in globalenv(), but that's not really sustainable.

@lionel-
Copy link
Member Author

lionel- commented Apr 30, 2021

Sort of off-topic, but still a legit part of my real-world road testing of this:
I see these warnings (a lot, i.e. every time I load_all(), test(), etc.):

You can update ellipsis to the latest CRAN version (from yesterday) to resolve the warnings. I'll look into a user prompt to update ellipsis for users who end up with new rlang and old ellipsis.

@lionel-
Copy link
Member Author

lionel- commented Apr 30, 2021

@jennybc The interactive development issue should be solved, we now look through the search path for the flag when devtools is detected.

@jennybc
Copy link
Member

jennybc commented May 1, 2021

Everything you say is true re: updates and changes here. It's nice! I'll have some more feedback soon.

In the meantime, can you bump version here so it's higher than CRAN, given the recent rlang release?

@lionel-
Copy link
Member Author

lionel- commented May 2, 2021

Done, we're now at 0.4.11.9000.

@jennybc
Copy link
Member

jennybc commented May 3, 2021

I have a fully-switched over version of googledrive now that uses this PR to confer cli-styling on all errors (and warnings), emitted with rlang::abort(): tidyverse/googledrive#346. That's still a draft and not finished, but it's definitely enough for me to make a few observations. Over all, this is already highly successful!

I notice some friction and dilemmas, which I outline here. Below, I'll amplify some of this with concrete examples:

  1. Feels a bit weird and asymmetric to call rlang::abort() and let rlang call cli for errors / warnings, but then do the opposite for messages (call cli::cli_bullets() and let cli call rlang::inform(). Who's in charge?
  2. Consistent application of cli theme tweaks. Currently I call cli::cli_bullets() via a wrapper, drive_bullets(). I tweak some cli styling there. How am I supposed to get these tweaks to also apply to rlang::abort() calls originating in googledrive?
  3. rlang's cli switch is sort of "hard to reach". It's awkward to talk about and demo rlang + cli, with the current method of activation = defining something in the package namespace.
  4. Programatically generating bullets from R objects is A Thing™️
  5. Message truncation and wrapping. I sometimes see that rlang::abort() truncates messages that would be displayed in full by cli::cli_bullets(). This seems like it might only affect interactive work in the Console, but not rendering an .Rmd? Also, in interactive work, it feels like I get into a state where rlang is not wrapping to exactly the same width as cli. Again, this is hard to repro, but I'm also sure this happens.

Consistent application of cli theme tweaks

Here's my wrapper around cli::cli_bullets():

drive_bullets <- function(text, .envir = parent.frame()) {
  quiet <- drive_quiet() %|% is_testing()
  if (quiet) {
    return(invisible())
  }
  # TODO: these tweaks currently don't apply to abort() calls, but should
  cli::cli_div(theme = list(
    span.field = list(transform = single_quote_if_no_color),
    span.val = list(color = NULL)
  ))
  cli::cli_bullets(text = text, .envir = .envir)
  cli::cli_end()
}

single_quote_if_no_color <- function(x) quote_if_no_color(x, "'")
double_quote_if_no_color <- function(x) quote_if_no_color(x, '"')

quote_if_no_color <- function(x, quote = "'") {
  if (cli::num_ansi_colors() > 1) {
    x
  } else {
    paste0(quote, x, quote)
  }
}

I think package developers will, in general, need to tweak cli styling, even if the specifics above aren't compelling.

There needs to be a way to specify these tweaks in one place and have them influence errors, warnings, and informational messages.


rlang's cli switch is sort of "hard to reach"

I have an internally-facing article on messaging in googledrive and I have to define .rlang_use_cli_format <- TRUE in a hidden chunk, in the global workspace, so my rlang::abort() calls use cli.

Is a good long-term solution, when someone needs to do a small demo of rlang with cli switched on?

The answer to this feels possibly linked to the other dilemmas, suggesting we might need a proper declaration for how a package uses cli and both rlang and cli consult it.


Programatically generating bullets from R objects is A Thing™️

I have extensive need to turn a character vector or a dribble into bullet points in googledrive. We @lionel- and @gaborcsardi have already talked about this on Slack. Here's my current solution, in the absence of rlang/cli tooling.

What needs to happen:

  • Map a cli-using string template over the object to get a character vector
  • Truncate this vector in an aesthetically pleasing way
  • Apply names to this vector to get the desired bullet points

map_cli() is an internal generic that turns an object into a vector of strings with cli markup.
Currently map_cli() has methods for character and dribble (and NULL and a default).

By default, each element of a character vector is styled as .field, but you can also pass a template.

map_cli(letters[1:3])
#> [1] "{.field a}" "{.field b}" "{.field c}"

map_cli(letters[4:6], template = "how about a path {.path <<x>>}?")
#> [1] "how about a path {.path d}?" "how about a path {.path e}?"
#> [3] "how about a path {.path f}?"

Note that we use non-standard glue delimiters (<< and >>, by default), because we are interpolating into a string with glue/cli markup, where {} has the usual meaning.

The map_cli.dribble() method makes a cli-marked up string for each row of the dribble, i.e. for each Drive file.

dat <- drive_find(n_max = 5)
map_cli(dat)
#> [1] "Rlogo.pdf {cli::col_grey('<id: 1opPZyo4WUiue56qlF8SO5ipaTJwPLzZg>')}"          
#> [2] "THANKS {cli::col_grey('<id: 1JCQ06Wj6AjntiyjnHsXmmcHZWCkBNgBJ>')}"             
#> [3] "googledrive-NEWS.md {cli::col_grey('<id: 1wMnFwFSIG0eQ4UUUEbn6-udF3GoKY3xN>')}"
#> [4] "def {cli::col_grey('<id: 12xjsE3eIf84VSFF5hNhSEOnQF8DDSHkc>')}"                
#> [5] "abc {cli::col_grey('<id: 1qkS667DEnvDzTuVBSyj5NeGzIGbrAy9Q>')}"

The result of map_cli() then gets passed to bulletize(), which adds the bullet-specifying names and does aesthetically pleasing truncation.

bulletize(map_cli(letters))
#>               *               *               *               *               * 
#>    "{.field a}"    "{.field b}"    "{.field c}"    "{.field d}"    "{.field e}" 
#>                 
#> "… and 21 more"

bulletize(map_cli(letters), bullet = "x", n_show = 2)
#>               x               x                 
#>    "{.field a}"    "{.field b}" "… and 24 more"

drive_bullets(c(
  "These are surprising things:",
  bulletize(map_cli(letters), bullet = "!")
))
#> These are surprising things:
#> ! 'a'
#> ! 'b'
#> ! 'c'
#> ! 'd'
#> ! 'e'
#>   … and 21 more

dat <- drive_find(n_max = 10)

drive_bullets(c(
  "Some Drive files:",
  bulletize(map_cli(dat))
))
#> Some Drive files:
#> • Rlogo.pdf <id: 1opPZyo4WUiue56qlF8SO5ipaTJwPLzZg>
#> • THANKS <id: 1JCQ06Wj6AjntiyjnHsXmmcHZWCkBNgBJ>
#> • googledrive-NEWS.md <id: 1wMnFwFSIG0eQ4UUUEbn6-udF3GoKY3xN>
#> • def <id: 12xjsE3eIf84VSFF5hNhSEOnQF8DDSHkc>
#> • abc <id: 1qkS667DEnvDzTuVBSyj5NeGzIGbrAy9Q>
#>   … and 5 more

It would be great if rlang/cli offered more support for generating 1 bullet point for each "observation" in an R object, to borrow some vctrs vocabulary.


Message truncation and wrapping

I'll come back with some examples later. But I am definitely having some experiences where the same vector of bullet points is wrapped differently when emitted from rlang::abort() vs. cli::cli_bullets(). I've also seen snapshots where the wrapping differs between an interactive test_file() call in the Console vs. a full test run via test() in the RStudio Build pane. And I have also seen surprising message truncation with rlang::abort().

@lionel-
Copy link
Member Author

lionel- commented May 4, 2021

Thanks for the thorough feedback Jenny!

Feels a bit weird and asymmetric to call rlang::abort() and let rlang call cli for errors / warnings, but then do the opposite for messages (call cli::cli_bullets() and let cli call rlang::inform(). Who's in charge?

I think in this approach you'd use inform() instead of cli_bullets(). Actually it seems like cli_bullets() should be named cli_inform()? I.e. it's one of the 3 signal functions cli_abort(), cli_warn(), cli_inform().

rlang's cli switch is sort of "hard to reach"

Defining the flag in the current topenv could be wrapped in a function rlang::use_cli_format() that could be called in a script or in a package namespace.

Programatically generating bullets from R objects is A Thing™️

It feels weird to generate strings containing interpolation syntax. I see interpolation as something that happens "here and now", rather than something that ends up quoted and processed later on. Do we need this laziness here? Or could we use something like cli_glue() to interpolate early with cli syntax?

Relatedly, are there functions in cli to generate each sort of markup? For instance ’cli_format_path()`. It would be helpful to also have a compat file with variants of such helpers to conditionally format elements using cli, if available. We'd use that in rlang and vctrs for consistent formatting.

@gaborcsardi
Copy link
Member

I think in this approach you'd use inform() instead of cli_bullets(). Actually it seems like cli_bullets() should be named cli_inform()? I.e. it's one of the 3 signal functions cli_abort(), cli_warn(), cli_inform().

I was to write the opposite, actually. The main reason for this is the status bar and progress bars. If cli knows that there is a status/progress bar in the bottom row of the terminal, then it will emit messages above it, instead of overwriting it, like a regular cat() or message().

So I would say, that if you use cli in your package and you also use rlang, then use abort() and warn() from rlang, but do the rest of the messaging with cli functions only.

It feels weird to generate strings containing interpolation syntax. I see interpolation as something that happens "here and now", rather than something that ends up quoted and processed later on.

I don't think this is the case here. I think the generated strings do not have "real" interpolation markup, but they have cli styling. I.e. {.class } markup, which only happens before cli emits the final messages.

Relatedly, are there functions in cli to generate each sort of markup? For instance ’cli_format_path()`. It would be helpful to also have a compat file with variants of such helpers to conditionally format elements using cli, if available. We'd use that in rlang and vctrs for consistent formatting.

The current cli architecture is more complex, for better and worse. You cannot replace it with a set of helper functions like that and get the same formatting.

@gaborcsardi
Copy link
Member

gaborcsardi commented May 4, 2021

  1. Feels a bit weird and asymmetric to call rlang::abort() and let rlang call cli for errors / warnings, but then do the opposite for messages (call cli::cli_bullets() and let cli call rlang::inform(). Who's in charge?

As I said above, no need to call rlang::inform(), it is actually better not to call it, or any other cat() or message() functions.

If this is an issue in general then we can also have the "opposite" API, i.e. call rlang from a cli_abort() and cli_warn(). IDK if this would be even more confusing, but at least you could opt in gradually.

  1. Consistent application of cli theme tweaks. Currently I call cli::cli_bullets() via a wrapper, drive_bullets(). I tweak some cli styling there. How am I supposed to get these tweaks to also apply to rlang::abort() calls originating in googledrive?

Yes, this is essentially r-lib/cli#256 I believe.

  1. Programatically generating bullets from R objects is A Thing™️

Yes. cli's default inline collapsing works against the vectorization in glue, paste(), etc. I haven't yet figured out a good way to solve this. I'll look at your code. Maybe we just need some marker that you want to vectorizee over some interpolated variables.

  1. Message truncation and wrapping. I sometimes see that rlang::abort() truncates messages that would be displayed in full by cli::cli_bullets(). This seems like it might only affect interactive work in the Console, but not rendering an .Rmd? Also, in interactive work, it feels like I get into a state where rlang is not wrapping to exactly the same width as cli. Again, this is hard to repro, but I'm also sure this happens.

I am fairly sure that this is the message length limit of R, which is 1000 characters by default. This seems a lot, but cli may generate a lot of ANSI sequences, which make the emitted string longer.

There is an option to change the limit, but it is supposed to be an end-user option. We could still change it temporarily, until we print the error, and then change it back.

This is also related to r-lib/cli#188, in that cli currently generates many unnecessary ANSI sequences. This will improved at some point, mid-term.

@lionel-
Copy link
Member Author

lionel- commented May 4, 2021

The main reason for this is the status bar and progress bars. If cli knows that there is a status/progress bar in the bottom row of the terminal, then it will emit messages above it, instead of overwriting it, like a regular cat() or message().

So I would say, that if you use cli in your package and you also use rlang, then use abort() and warn() from rlang, but do the rest of the messaging with cli functions only.

Do we want all messages to have this behaviour? Or are there different behaviours appropriate for different situations? E.g. you'd keep cli_bullets() separate form cli_inform() for the cases of status bar vs no-status bar? Are there guidelines on choosing one type of message vs the other in that case?

I don't think this is the case here. I think the generated strings do not have "real" interpolation markup, but they have cli styling. I.e. {.class } markup, which only happens before cli emits the final messages.

I see, which is why to interpolate we need an extra pair of brackets inline.

The current cli architecture is more complex, for better and worse. You cannot replace it with a set of helper functions like that and get the same formatting.

There are some types of components that are very common in error messages, like types and argument names. Do we envision a consistent formatting for these elements or do we envision disparate formatting across packages? If the former we probably need to find a way to format these essential elements from lower-level packages which generate lots of errors, such as vctrs.

Alternatively we could have vctrs depend on cli and decide that rlang only issues low-level errors which are not user facing. That will not be fully achievable though, as rlang is in charge of creating r-lib lambdas for instance (used in purrr and dplyr). That's why I hope there is a way to get external formatting of these common error items.

@gaborcsardi
Copy link
Member

Do we want all messages to have this behaviour? Or are there different behaviours appropriate for different situations? E.g. you'd keep cli_bullets() separate form cli_inform() for the cases of status bar vs no-status bar? Are there guidelines on choosing one type of message vs the other in that case?

You don't know if there is a status bar or not, because another package might create one and then call your package.

@lionel-
Copy link
Member Author

lionel- commented May 4, 2021

My question was whether we want all messages to account for a potential status bar. Will messages going through inform() look out of place? If the answer is yes then probably inform() should do the right thing regarding cli integration?

@jennybc
Copy link
Member

jennybc commented May 4, 2021

So I would say, that if you use cli in your package and you also use rlang, then use abort() and warn() from rlang, but do the rest of the messaging with cli functions only.

I can live with this. The main missing thing is a way for me to capture the package-specific theme tweaks and direct both rlang and cli to use them.

Programatically generating bullets from R objects is A Thing™️
It feels weird to generate strings containing interpolation syntax. I see interpolation as something that happens "here and now", rather than something that ends up quoted and processed later on. Do we need this laziness here?

I think it's important to distinguish data interpolation and style realization. With my current (clunky) approach, I'm doing data interpolation early (because I have no choice at this point), but I'm delaying style realization. Many of my messages have 1 or more "regular" bullets, a middle section of bullets generated from an R object, then maybe a final "regular" bullet. Here's a real example, where I print a dribble in an unusual way, because it's more important to show MIME type than file id:

abort(c(
      "Only native Google files can be published.",
      "{.arg file} includes {?a/} file{?s} \\
       with non-native MIME type{cli::qty(nrow(file))}",
      bulletize(map_cli(file, "<<name>>: {.field <<mimeType>>}")),
      "i" = "You can use {.fun drive_share} to change a file's sharing \\
             permissions."
    ))

It feels more correct to hand abort() (or cli) a message (or text) vector where the elements are uniform, i.e. I'm only using the bulletize(map_cli(...)) thing to generate the strings I would have typed out by hand, if that were practical. To say it another way, it feels wrong to hand over a vector where some elements are raw, while others are fully baked.

Yes. cli's default inline collapsing works against the vectorization in glue, paste(), etc. I haven't yet figured out a good way to solve this. I'll look at your code. Maybe we just need some marker that you want to vectorizee over some interpolated variables.

Yeah this feels like a real rough edge, but also one that can be solved now that we recognize it. One part that feels hard/awkward is that currently our messages have a "one element per line" convention, which feels good overall. So bullet expansion isn't just a variation on inline collapsing. I guess we have an element like c("!" = "{.field {cli_magic(x)}}") that expands to the length of x? (Well, I think the truncation + "... and n more" thing is also usually needed.)

jennybc added a commit to tidyverse/googledrive that referenced this pull request May 4, 2021
A CRAN release happened while the r-lib/rlang#1176 PR has been in play
@jennybc
Copy link
Member

jennybc commented May 4, 2021

There are some types of components that are very common in error messages, like types and argument names. Do we envision a consistent formatting for these elements or do we envision disparate formatting across packages? If the former we probably need to find a way to format these essential elements from lower-level packages which generate lots of errors, such as vctrs.

I think one of the outcomes of all the recent messaging work should be to update this:

https://style.tidyverse.org/error-messages.html

with a lot more specifics around how to style various things, using cli (with and without color) and how to do it "by hand" if you must.

I'll stay out of the mechanics re: whether a compat file is realistic, but I think we can get very far with a clear table of house styles.

@lionel- lionel- merged commit bda055d into r-lib:master May 12, 2021
@lionel- lionel- deleted the cli-errors branch May 12, 2021 13:25
jennybc added a commit to tidyverse/googledrive that referenced this pull request May 25, 2021
* Use the rlang PR with cli-formatted messages

* New formatting functions (and reorder this file)

* Convert `stop()` and friends to `abort()`

Delete the older `stop()` helpers

* Actually install rlang from the PR, oops

* Mostly style

* Use `warn()`

* Switch to `map_cli()` + `bulletize()`

* Namespace `tail()`

* Basic doc for map_cli()

* Style column names with `.code`

I have no idea why the wrapping of other messages changed.

* Use `bulletize(map_cli())` everywhere possible

For now, at least, let's be internally consistent. But it is often more verbose :(

* Update the rlang version

A CRAN release happened while the r-lib/rlang#1176 PR has been in play

* OK, we're going back to the cli_abort() plan!

Go back to CRAN rlang, advance to bleeding edge cli

* Update GHA

* Add snapshot tests

* Use the ability to customize how dribbles are formatted

* Update the message() mask

* Switch to cli_abort(); add snapshot test

* Improve the messages re: weird stuff from drive_path_get()

* Another `name <- path` hack straggler

* File should never have been committed

* Update NEWS

* Introduce drive_abort(); unify styling with drive_bullets()

Make Drive file names (or, sometimes, paths) styled more like cli's .file style, e.g. quoting in certain situations

* Actually what I want

This seems to accomplish what I actually want, which is bullets are "normal" color (e.g. not red/orange, inside an error message).

* I guess we might as well do cli_warn() too

* Update the internal article

* No need to import these from cli

* Update WORDLIST

* Update snapshot
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.

Support glue in abort()
3 participants