-
Notifications
You must be signed in to change notification settings - Fork 39
Description
Context: I was trying to figure out why CI environments (i.e. CI=true and similar) weren't being colorized by default when using anstream to plumb output containing ANSI colors. I ended up on this choice implementation:
anstyle/crates/anstream/src/auto.rs
Lines 197 to 223 in cbad464
| #[cfg(feature = "auto")] | |
| fn choice(raw: &dyn RawStream) -> ColorChoice { | |
| let choice = ColorChoice::global(); | |
| match choice { | |
| ColorChoice::Auto => { | |
| let clicolor = anstyle_query::clicolor(); | |
| let clicolor_enabled = clicolor.unwrap_or(false); | |
| let clicolor_disabled = !clicolor.unwrap_or(true); | |
| if anstyle_query::no_color() { | |
| ColorChoice::Never | |
| } else if anstyle_query::clicolor_force() { | |
| ColorChoice::Always | |
| } else if clicolor_disabled { | |
| ColorChoice::Never | |
| } else if raw.is_terminal() | |
| && (anstyle_query::term_supports_color() | |
| || clicolor_enabled | |
| || anstyle_query::is_ci()) | |
| { | |
| ColorChoice::Always | |
| } else { | |
| ColorChoice::Never | |
| } | |
| } | |
| ColorChoice::AlwaysAnsi | ColorChoice::Always | ColorChoice::Never => choice, | |
| } | |
| } |
If my read of that helper is right, the current logic only considers anstyle_query::is_ci() after raw.is_terminal() is satisfied. In practice that means that ColorChoice::Always branch won't be taken for CI environments, since (AFAIK) most CI environments don't actually provide a terminal (they support ANSI color codes, but they won't provide a true pty that would pass is is_terminal() check).
Given that, does it makes sense to split that check out, so that is_ci() becomes a sufficient condition for ColorChoice::Always? My intuition is "yes" (since the CI check seems to be a no-op as is), but there's also some nuance given that CI environments aren't real terminals and might only support a small subset of the escapes normally accepted by terminals (things like OSC codes in particular).
So, I don't have a super clear sense here, but I figured I'd open this in case others have a clearer picture than I do 😅. If this change makes sense, I'd be happy to send a PR for it.