Reduce verbosity in getting the final color for a stream

[pksunkara]

With the updated crates and API:

    // I read the color flag the user specified in the cli arguments and default it for my program
    program.color.write_global();

    // I get the final result for the `stdout` stream of whether to show color or not
    let should_color = match anstream::AutoStream::choice(&std::io::stdout()) {
        anstream::ColorChoice::Auto => unreachable!(),
        anstream::ColorChoice::AlwaysAnsi => true,
        anstream::ColorChoice::Always => true,
        anstream::ColorChoice::Never => false,
    };

    // I tell the logger to show color
    tracing_subscriber::registry()
        .with(
            tracing_subscriber::fmt::layer()
                .with_ansi(should_color)
        )
        .init();

Before, it used to be:

    program.color.write_global();

    tracing_subscriber::registry()
        .with(
            tracing_subscriber::fmt::layer()
                .with_ansi(get(Stream::Stdout).color())
        )
        .init();

I think we can make it simpler similarly:

    program.color.write_global();

    tracing_subscriber::registry()
        .with(
            tracing_subscriber::fmt::layer()
                .with_ansi(stdout().color())
        )
        .init();

All we have to do is add pub fn color(&self) -> bool to anstream::AutoStream

[epage]

.with_ansi(stdout().color()) has a bug; stdout might support color but not ansi and passing it to a control that only supports ansi. As I mentioned on Zulip, there are multiple questions that might be asked and people need to make sure they are clearly picking the right one which is why I'm not in favor of simplifying this atm.

In my applications, I did this with just a matches

    let colored_stderr = !matches!(
        anstream::AutoStream::choice(&std::io::stderr()),
        anstream::ColorChoice::Never
    );

(this was passed to env_logger which uses termcolor for now which is why this specific code works)

[pksunkara]

We also had .ansi_color() in the old API.

Whatever you did, I see everyone needing to do it. That means, we need to simplify the verbosity of it.

[pksunkara]

Even simplifying the anstream::AutoStream::choice(&std::io::stderr()) to stderr().choice() to return ColorChoice is better, because we want the end-users of anstream to prefer using stdout() and stderr() directly in most common use cases instead of AutoStream.

And then we can add color_choice.is_never() for matches!(color_choice, ColorChoice::Never) on top of it. This way we just reduce the verbosity without making any assumptions about the logic.

[epage]

Whatever you did, I see everyone needing to do it. That means, we need to simplify the verbosity of it.

First, anstream is not concolor and its framing is not around answering questions about the terminal. This means we do not need to primarily design around answering questions about the terminal. We are providing them with information on what choice we would make and they can then decide what they want to do with that. For where this API sits, that seems like the right decision.

Even simplifying the anstream::AutoStream::choice(&std::io::stderr()) to stderr().choice() to return ColorChoice is bette

That is the natural choice but I ended up not going with it in #75 which I unfortunately did not document. I think its two parts

• This forces a move and I think I had cases where I didn't want to move due to generics
• The function is returning what choice we will make, not what the current stream is. This is a subtle distinction for the Windows case (we return Always rather than AlwaysAnsi). To even answer the question we currently are, we'd need to pass the choice down into the type

[pksunkara]

To even answer the question we currently are, we'd need to pass the choice down into the type

I have seen you mention this more than once, but I am still quite confused about what you mean. Can you please give more details?

---

What about the color_choice.is_never() part of my comment?

[epage]

I have seen you mention this more than once, but I am still quite confused about what you mean. Can you please give more details?

anstream::AutoStream::choice resolves Auto to Always or Never, and not AlwaysAnsi. AutoStream::always then determines what should happen on windows (ansi or not).

A naive implementation of asking an AutoStream instance what its color choice is is

match self {
    StreamInner::PassThrough(_) => AlwaysAnsi,
    StreamInner::Strip(_) => Never,
    #[cfg(all(windows, feature = "wincon"))]
    StreamInner::Wincon(_) => Never,
}

which returns a different result than AutoStream::choice in the PassThrough case. We don't know if it was originally Always or AlwaysAnsi, so we can only return AlwaysAnsi as that is the most correct choice.

If we wanted to have the same behavior of AutoStream::choice, we'd need to take choice and store it inside AutoStream so we could return it.

[pksunkara]

If we wanted to have the same behavior of AutoStream::choice, we'd need to take choice and store it inside AutoStream so we could return it.

Yup, when I was proposing the change to the API, I assumed the implementation would change to this too. I should have clarified it more.

What are the objections for this new implementation?

[epage]

I remember it was a very conscious choice but I do not remember what all went into it. My focus is currently elsewhere for the current time so I'll just leave it at that rather than distract myself with having to come back up to speed on it.

[pksunkara]

I will try to make a PR then and see if anything pops up.