r-lib / cli

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

Inconsistent behaviour in cli::cli_inform() #682

Closed serkor1 closed 2 months ago

serkor1 commented 3 months ago

The rlang::inform()-wrapper in the cli-package have a different behavior than using it directly, see the two examples below,

Using rlang::inform()

rlang::inform(
  message = cli::rule(
    left  = "left",
    right = "right"
  )
)
── left ─── right ──

Using cli::cli_inform()

cli::cli_inform(
  message = cli::rule(
    left  = "left",
    right = "right"
  )
)
── left ───── right
──

If I only specify left, the following happens,

cli::cli_inform(
  message = cli::rule(
    left  = "left"
  )
)
── left 
─────

This behavior is, as far as my tests go, independent of console_width(). Am I using cli::cli_inform incorrectly, or is this a bug?

gaborcsardi commented 2 months ago

I don't know if this is a bug, cli::cli_inform() wraps the next, rlang::inform() does not. Your rule is exactly as wide as the terminal width, so it will be wrapped, because of wrapper is conservative and does not create lines that are as wide as the console width.

The root of the issue is that rule() does not play very well with cli_inform() and similar functions, because rule() assumes no wrapping. You could create a shorter rule, but that might fail (= get wrapped) as well, e.g. if there is indentation or a label in cli_inform():

❯ cli::cli_bullets(c("!" = cli::rule(left = "left")))
! ── left
  ───────────────────────────
❯ cli::cli_bullets(c("!" = cli::rule(left = "left", width = cli::console_width() - 2)))
! ── left
  ─────────────────────────
serkor1 commented 2 months ago

I see your point - however, a fair expectation is that the wrapper function behaves as the wrapped function, with less arguments. The rlang::inform()-function has no issues dealing with cli::rule(), at all.

And considering that there is an interest in using cli for packageStartupMessage instead of using rlang directly, I think it would be a great feature that we can get consistent behavior between these two functions!


Also, I love this library - and its just an awesome feature that I think should be implemented, honestly! 💯

gaborcsardi commented 2 months ago

Neither functions are wrappers of the other, they are actually quite different. E.g.

options(cli.width = 30, width = 30)
txt <- cli:::lorem_ipsum(1, 4)
cli::cli_inform(txt)
Commodo et deserunt et dolor
enim aliqua tempor pariatur.
Tempor anim nisi commodo
labore nostrud do dolore non
enim pariatur ad incididunt.
Enim cupidatat nisi esse sit
fugiat enim voluptate sit
consectetur incididunt ex.
rlang::inform(txt)
Commodo et deserunt et dolor enim aliqua tempor pariatur. Tempor anim nisi commodo labore nostrud do dolore non enim pariatur ad incididunt. Enim cupidatat nisi esse sit fugiat enim voluptate sit consectetur incididunt ex.

Also perhaps most importantly:

cli::cli_inform("letters: {letters[1:5]}")
letters: a, b, c, d, and e
rlang::inform("letter{?s}: {letters[1:5]}")
letter{?s}: {letters[1:5]}
serkor1 commented 2 months ago

This is an interesting comparison, thank you for clarifying the different behaviour. I was referring to your code below,

https://github.com/r-lib/cli/blob/bcb5c78a20122d62b9c84a0bfbf09900fe928036/R/rlang.R#L67-L72

and labelled it as a wrapper-function. When reading the documentation I get the idea that its essentially rlang::inform() with all the cool features of cli.

But I take from your examples and comments that I am wrong about this, yes?

gaborcsardi commented 2 months ago

with all the cool features of cli.

format_message() does a lot of things there, though. E.g. it wraps the lines, which means that it will not play well with cli::rule().

serkor1 commented 2 months ago

Yeah, I made a fork to check it out - way above my skills! 🤣 Ok, Ill leave this issue then. If you ever decide to add something like the above, without the wraps then know that I, for one, would be over-joyous!

Amazing work, honestly!

gaborcsardi commented 2 months ago

In the next major version of cli you'll be able to embed cli_rule() into cli_bullets(), etc.

serkor1 commented 2 months ago

So I can essentially use cli for packageStartupMessages with cli::rule()?