Colours shouldn't depend on environment variables or stderr

[Timmmm]

What happened?

Currently prql has the inadvisable behaviour that the presence of ANSI colour codes in the output for errors depends on NO_COLOR and maybe if stderr is a tty (I didn't look into the details). This is obviously not the right thing to do, but it seems like this was a temporary backwards compatibility measure:

fn should_use_color() -> bool {
    match anstream::AutoStream::choice(&stderr()) {
        anstream::ColorChoice::Auto => true,
        anstream::ColorChoice::Always => true,
        anstream::ColorChoice::AlwaysAnsi => true,
        anstream::ColorChoice::Never => false,
    }
}

/// Strip colors, for external libraries which don't yet strip themselves, and
/// for insta snapshot tests. This will respond to environment variables such as
/// `CLI_COLOR`. Eventually we can remove this, always pass colors back, and the
/// consuming library can strip (including insta
/// https://github.com/mitsuhiko/insta/issues/378).
pub fn strip_colors(s: &str) -> String {
    if !should_use_color() {
        strip_str(s).to_string()
    } else {
        s.to_string()
    }
}

So this is just a note to actually remove this "eventually" :-D

Tbh I don't really understand why you deprecated the colors option. Seems like a very useful thing?

PRQL input

NA

SQL output

NA

Expected SQL output

No response

MVCE confirmation

• Minimal example
• New issue

Anything else?

No response

[max-sixty]

Yes, I think there were lots of tradeoffs here. We decided to go with the philosophy at https://docs.rs/anstream/0.6.11/anstream/ & rust-cli/concolor#47 (possibly there's a fuller description somewhere but I couldn't immediately find it)

i.e.

• the library always pass text with colors back
• consuming code can choose to strip the codes

Worth noting that prqlc the binary does auto detect whether the output is a tty, because we delegate writing to anstream too.

Tbh I don't really understand why you deprecated the colors option. Seems like a very useful thing?

Lots of tradeoffs — the downside is that every function requires an additional param, and we need to thread those params through the every library call...

---

Happy to leave this open to solicit and thoughts or questions; eventually we can mark as postponed and come back to it when libraries such as insta have added something to strip the codes

[max-sixty]

I recently worked on the code in ClickHouse for its native PRQL support. I added something to strip color codes, since PRQL doesn't do that any longer.

I worry that removing that option was too strict. While the ideology of anstream resonates — i.e. don't have every function passing color all the way through the function chain, just strip them at the point of use — by not offering the option, we force others to do this. Lots of tools don't want the color and don't really want to strip the codes themselves.

So possibly we add back re-enable color support in bindings' API. We can generate colors throughout our code — we're not going to add back the option in the whole function chain — but at the final return site, we can strip them if color: false is passed. This is basically a convenience so users of the API don't have to do it themselves.