r-lib / cli

Tools for making beautiful & useful command line interfaces
https://cli.r-lib.org/
Other
625 stars 66 forks source link

`cli_warning` and `cli_inform` don't expose individual bullet points in condition object. #666

Closed plietar closed 2 weeks ago

plietar commented 5 months ago

The cli_warning and cli_inform functions work by formatting their arguments as a single string, which then gets passed on to rlang::warn/rlang::inform

Unfortunately this means that when inspecting the produced condition objects programmatically (using testthat::expect_warning for example), the bullet points cannot be accessed individually, which makes it harder to write assertions against.

This behaviour is inconsistent with cli_abort, which instead formats each bullet point separately and passes a vector to rlang::error. By doing this, the main message is available in the condition object as $message, and the rest of the bullet points are available as $body.

See below the difference between cli_abort, whose additional bullets are available as e$body, and warnings.

e <- expect_error(cli_abort(c("Main message", i="Extra information")))
e$message
#>                
#> "Main message"
e$body
#>                   i 
#> "Extra information"

w <- expect_warning(cli_warn(c("Main message", i="Extra information")))
w$message
#> [1] "Main message\nℹ Extra information"
w$body
#> NULL

If the implementation of cli_warning error was closer to cli_abort then the bullet points would be available in the warning object.

w <- expect_warning(
  rlang::warn(c(
    format_inline("Main message"),
    i=format_inline("Extra information")),
    use_cli_format = TRUE))
w$message
#>                
#> "Main message"
w$body
#>                   i 
#> "Extra information"

To be honest this seems like an easy fix by copying the implementation of cli_abort, and I'd be happy to make a PR for it. However I feel there might be a reason for the inconsistent behaviour that I am missing.

gaborcsardi commented 5 months ago

If you need extra information in the error object, then you can add that directly, and possibly create a new error class. It does not seem like a good idea to try to infer information from the message. E.g. error and warning messages might be translated in the future.

plietar commented 5 months ago

I understand that trying to save useful data into the error's fields is a bit of a code smell and not a good idea.

In this case however I'm only trying to write some testthat assertions against the message, using expect_equal/expect_match. It is nicer to write one assertion for the main message and one for the bullet points than a single one for the entire message.

Regardless, it is also surprising (to me at least) that cli_abort and cli_warn behave differently when it comes to wrapping up the message.

gaborcsardi commented 5 months ago

I suppose it would make sense to make them behave the same way, but it is not entirely clear at this point whether that'll break existing code.

In any case, whatever is in message and in body is an implementation detail. The correct API to get the full message is conditionMessage():

❯ conditionMessage(e)
[1] "\033[1m\033[22mMain message\n\033[36mℹ\033[39m Extra information"

❯ conditionMessage(w)
[1] "\033[1m\033[22mMain message\n\033[36mℹ\033[39m Extra information"

I think you can use that to match your error message, with a constant or Inf line width (see ?testthat::local_reproducible_output).

Personally, I would use expect_snapshot() for this, without conditionMessage().

gaborcsardi commented 2 weeks ago

I am going to close this, as it seems that there is nothing actionable.