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

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

Closed
plietar opened this issue Jan 22, 2024 · 4 comments

Comments

@plietar
Copy link

plietar commented Jan 22, 2024

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
Copy link
Member

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
Copy link
Author

plietar commented Jan 29, 2024

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
Copy link
Member

gaborcsardi commented Jan 29, 2024

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
Copy link
Member

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

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

No branches or pull requests

2 participants