Migrate to Anstyle #7
Conversation
|
@lalrpop/maintainers Looking for a review and also noting that I don't seem to have write access despite being a lalrpop member... that's fine but hopefully someone active does for when this is good to go. |
|
Will close #4 and a pre-req for lalrpop/lalrpop#856 |
|
|
||
| macro_rules! fg_color { | ||
| ($color:expr, $term_color:ident) => { | ||
| if self.contains($color) { | ||
| if term.supports_color() { |
There was a problem hiding this comment.
I think this functionality is lost in this PR. That is, if the thing we output to isn't a proper terminal (a file), we used to not add any styling, while we probably now always output ANSI escape codes - which can be annoying when redirecting stderr to a log file.
The simplest approximation is to use a bound std::io::Write + std::io::IsTerminal, and use is_terminal() as a replacement for supports_color() (I don't know if it's an exact replacement but I've seen is_terminal being used for that in practice).
There was a problem hiding this comment.
The approach you mention is also the official recommendation of the termcolor library, which seems like a relevant endorsement: https://docs.rs/termcolor/latest/termcolor/#detecting-presence-of-a-terminal
There was a problem hiding this comment.
Ah, fair point, I didn't know about std::io::IsTerminal. I think for Lalrpop, the functionality was maintained because there I started using AutoStream/StripStream which manage this for me. I pushed the change that has been suggested here. It does break my lalrpop implementation because AutoStream/StripStream don't implement std::io::IsTerminal. It might be related to IsTerminal being a sealed trait? Some trait shenanigans I think.
Maybe I don't need Anstream on the Lalrpop side if all of the style/color checking can happen here?
|
@Pat-Lafon you (and other @lalrpop/maintainers) should now have write access, sorry about that |
This new version has some bugfixes and improvements, and greatly reduces the transitive dependencies. Dropping the term dependency entirely (lalrpop#7) is still something we should do, but this is an easy improvement in the meantime.
This is a first pass at moving away from
term. It compiles but I haven't actually looked too much into what styling happens.One nice thing is that now many of the bounds are now based on the standard library like
std::io::{Write, Result}instead ofTerminal + ?Sizedorterm::Result.The way styles are applied is a little different, but all of the colors and most of the styles are basically the same. Note
Standoutwhich i think is equivalent toanstyle::Effects::BOLD | anstyle::Effects::INVERTbut not 100% clear on that.This crate has very few tests which makes these kinds of updates a little risky imo though I don't think the diff is too big that we can't go through it thoroughly and maybe add some
instasnapshot testing.