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

.ansi_color() equivalent? #105

Closed
max-sixty opened this issue Jun 8, 2023 · 8 comments
Closed

.ansi_color() equivalent? #105

max-sixty opened this issue Jun 8, 2023 · 8 comments

Comments

@max-sixty
Copy link

max-sixty commented Jun 8, 2023

I'm trying to move PRQL over to anstream, in PRQL/prql#2773

(This is based on rust-cli/concolor#47, and having coincidentally struggled to use a more global approach a few days ago in PRQL/prql#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
Copy link
Collaborator

epage commented Jun 9, 2023

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 added a commit to max-sixty/prql that referenced this issue Jun 9, 2023
@max-sixty
Copy link
Author

Great — I'm trying that new approach in PRQL/prql#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
Copy link
Collaborator

epage commented Jun 9, 2023

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

  • Disable colors in the tests (I think I saw that in one case)
  • Use a StripStream
  • Use the underlying strip_str

@max-sixty
Copy link
Author

strip_str is perfect! Thanks.

@epage
Copy link
Collaborator

epage commented Jun 9, 2023

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

Nice!

Though for a library like insta — which has no UI and just wants to strip color codes — mitsuhiko/insta#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
Copy link
Collaborator

epage commented Jun 9, 2023

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

Dependency trees:

  • strip-ansi-escapes
    • vte
      • utf8parse
      • vte_generate_state_changes (proc-macro)
        • proc_macro2
        • quote
  • anstream (disabling features)
    • anstyle
    • anstyle-parse (this is a stripped down fork of vte)
    • utf8parse

@max-sixty
Copy link
Author

Awesome!

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