rust-cli / anstyle

ANSI text styling
https://docs.rs/anstyle
Other
106 stars 16 forks source link

`.ansi_color()` equivalent? #105

Closed max-sixty closed 1 year ago

max-sixty commented 1 year ago

I'm trying to move PRQL over to anstream, in https://github.com/PRQL/prql/pull/2773

(This is based on https://github.com/rust-cli/concolor/issues/47, and having coincidentally struggled to use a more global approach a few days ago in https://github.com/PRQL/prql/pull/2755)

Is there an equivalent to .ansi_color()? For a dependency that doesn't itself use anstream, I was intending to pass something down.

Or am I thinking about this the wrong way? should we pass true down and then use anstream's filtering in our crate? Feel free to point me at docs if I've missed them...

Currently I'm doing this, but it doesn't cover the Auto case:

https://github.com/PRQL/prql/pull/2773/files#diff-939acd3d8f24e19ce7ac0573190c87b1a63001bb386c20f7b36b3cf50e65a9edR290-R297

        // TODO: Ideally we would remove passing `color` down completely, and
        // rely on the global env & anstream's filtering.
        let color = match anstream::ColorChoice::global() {
            anstream::ColorChoice::Always => true,
            anstream::ColorChoice::Never => false,
            anstream::ColorChoice::AlwaysAnsi => true,
            anstream::ColorChoice::Auto => color,
        };

(You're of course welcome to critique the PR over there, but not expecting you to review every anstream PR...)

epage commented 1 year ago

Is there an equivalent to .ansi_color()? For a dependency that doesn't itself use anstream, I was intending to pass something down.

Or am I thinking about this the wrong way? should we pass true down and then use anstream's filtering in our crate? Feel free to point me at docs if I've missed them...

There is AutoStream::choice which I sometimes use for some minor optimizations or interop with other libraries, like env_logger (hmm, looks like I still have some concolor code sitting around...).

but you do have the right idea that where ever you can pass the anstream::stdout() or anstream::stderr() (maybe with calling lock() as well), you should prefer that. The primary goal is to decouple output formatting from terminal capabilities. Clap is a great example where people want to provide a colored Command::after_help and then clap will call into anstream::stdout and The Right Thing will be done.

max-sixty commented 1 year ago

Great — I'm trying that new approach in https://github.com/PRQL/prql/pull/2773 now. My manual tests for printing to the terminal work well.

But is there a recommended approach for tests? We have snapshot tests on the string representation of our errors. And they're trying to write a garbled mess :). For example, at https://github.com/PRQL/prql/actions/runs/5217528263/jobs/9417456918?pr=2773#step:11:270

-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    0       │-Error:␊
    1       │-   ╭─[:2:20]␊
    2       │-   │␊
    3       │- 2 │     let a = [1, 2, false]␊
    4       │-   │                    ──┬──␊
    5       │-   │                      ╰──── array expected type `int`, but found type `bool`␊
    6       │-───╯
          0 │+␛[31mError:␛[0m␊
          1 │+   ␛[38;5;246m╭␛[0m␛[38;5;246m─␛[0m␛[38;5;246m[␛[0m:2:20␛[38;5;246m]␛[0m␊
          2 │+   ␛[38;5;246m│␛[0m␊
          3 │+ ␛[38;5;246m2 │␛[0m ␛[38;5;249m ␛[0m␛[38;5;249m ␛[0m␛[38;5;249m ␛[0m␛[38;5;249m ␛[0m␛[38;5;249ml␛[0m␛[38;5;249me␛[0m␛[38;5;249mt␛[0m␛[38;5;249m ␛[0m␛[38;5;249ma␛[0m␛[38;5;249m ␛[0m␛[38;5;249m=␛[0m␛[38;5;249m ␛[0m␛[38;5;249m[␛[0m␛[38;5;249m1␛[0m␛[38;5;249m,␛[0m␛[38;5;249m ␛[0m␛[38;5;249m2␛[0m␛[38;5;249m,␛[0m␛[38;5;249m ␛[0mfalse␛[38;5;249m]␛[0m␊
          4 │+ ␛[38;5;240m  │␛[0m                    ──┬──␊
          5 │+ ␛[38;5;240m  │␛[0m                      ╰──── array expected type `int`, but found type `bool`␊
          6 │+␛[38;5;246m───╯␛[0m␊

Is there some "Write to String" approach?

epage commented 1 year ago

Not quite sure what the underlying test looks like to say but if you don't want to verify the colored output then you can

max-sixty commented 1 year ago

strip_str is perfect! Thanks.

epage commented 1 year ago

btw in case someone brings up the entrenched strip-ansi-escapes, this is a benchmark from stripping the output from rg --color always -i linus within the Linux source code (1000 lines with 2 colored sections per line)

rg_linus.vte/strip_ansi_escapes
                        time:   [1.8652 ms 1.8676 ms 1.8702 ms]
rg_linus.vte/advance_strip
                        time:   [496.71 µs 497.43 µs 498.20 µs]
rg_linus.vte/strip_str  time:   [198.52 µs 198.76 µs 199.02 µs]
rg_linus.vte/StripStr   time:   [183.69 µs 185.06 µs 186.19 µs]
rg_linus.vte/strip_bytes
                        time:   [207.02 µs 208.59 µs 209.91 µs]
max-sixty commented 1 year ago

Nice!

Though for a library like insta — which has no UI and just wants to strip color codes — https://github.com/mitsuhiko/insta/issues/378 — it is nice to have a very small dependency which only does the stripping. We can see what folks think there re the concept + the dependency size. (Or let me know if I'm thinking about this badly...)

epage commented 1 year ago

I expect strip-ansi-escapes to have worse build times

Dependency trees:

max-sixty commented 1 year ago

Awesome!