tealdeer-rs / tealdeer

A very fast implementation of tldr in Rust.
https://tealdeer-rs.github.io/tealdeer/
Apache License 2.0
4.17k stars 123 forks source link

Replace ansi_term #288

Closed dbrgn closed 2 years ago

dbrgn commented 2 years ago

ansi_term is not actively maintained anymore (see https://rustsec.org/advisories/RUSTSEC-2021-0139). Replace it with a suitable alternative.

I looked at termcolor, owo-colors and yansi, and ended up picking yansi:

The only change from a config API point of view is that "purple" is now renamed to "magenta", but "purple" still works.

(Based on #287, merge that one first!)

dbrgn commented 2 years ago

Note: We could replace the custom enable_colors logic with Paint::disable(), but we'd have to test that it doesn't have unintended interactions with our tests (due to global state).

niklasmohrin commented 2 years ago

Beyond tests: Does the disable functionality not have any performance impact? All methods that actually produce bytes from a Paint have to load from an atomic boolean before deciding how to paint :thinking: Let's measure before merging

dbrgn commented 2 years ago

Yes, that's true. I suspect there's no significant impact on real-world use cases like rendering a tldr page. I'm fairly sure the overhead will be insignificant compared to the I/O we're doing. However, I'll try to measure it.

dbrgn commented 2 years ago
[main] $ cargo build --release
[main] $ mv target/release/tldr tldr-ansi-term
[main] $ git switch replace-ansi-term
[replace-ansi-term] $ cargo build --release
[replace-ansi-term] $ mv target/release/tldr tldr-yansi

...and then:

screenshot-20220925-174600

If you run the benchmark multiple times, you get varying results. I'd say that the change is not significant. (I didn't expect it to be.)

niklasmohrin commented 2 years ago

Okay, I also reproduced these performance results :+1:

niklasmohrin commented 2 years ago

I wish they had Cargo features to remove a) the wrapping and b) the enable/disable stuff because I don't want to think about that :D

niklasmohrin commented 2 years ago

About using disable: I think I had some thoughts about OutputManager some time back to better separate concerns with IO, but never came to implement / design it to the end

dbrgn commented 2 years ago

I wish they had Cargo features to remove a) the wrapping and b) the enable/disable stuff because I don't want to think about that :D

Feel free to open an issue on the yansi issue tracker 🙂