Global enable/disable encourages the wrong behavior

[epage]

Global enable/disable encourages the wrong behavior

[SergioBenitez]

If I understand correctly, you're worried about a scenario like the following:

1. User check if stdout is a tty, calls Paint::disable() if not.
2. User logs to stderr with colors, now disabled.
3. stderr is actually a tty that supports colors, but colors were disabled.

Is that right? I agree this is an unfortunate circumstance, though I would argue that it is rooted in user error. Nevertheless, what would you propose yansi do?

[epage]

I'm less worried with the user calling Paint::disable when the stream they check isn't a TTY but am more concerned with skipping Paint::disable when they stream they check is a TTY but the user has the other stream sent to a file.

Is that right? I agree this is an unfortunate circumstance, though I would argue that it is rooted in user error. Nevertheless, what would you propose yansi do?

User error is commonly a manifestation of a systemic failure that led to the user error. yansi gives users a tool that is overly broad and doesn't provide any guidance on when to use it / not use it.

Idea 1: My automatic reaction is to ask "why does yansi provide disable"? I can see using it combined with something like enable_windows_ascii but if the user is doing TTY detection, they already need to handle the streams differently on their own, so the value gained by disable is relatively low.

Idea 2: If there is a motivating case for disable, my next thought is that yansi should document when to use it / not use it and why in hopes people notice the documentation.

Idea 3: A bit more experimental of an idea is, like a truecolor constrain function in #15, what if there were stdout and stderr functions on Style that checked individual bools.

This would turn the README's example from

print!("{} light, {} light!", Paint::green("Green"), Paint::red("red").underline());

to

print!("{} light, {} light!", Paint::green("Green").stdout(), Paint::red("red").underline().stdout());

[SergioBenitez]

I'm less worried with the user calling Paint::disable when the stream they check isn't a TTY but am more concerned with skipping Paint::disable when they stream they check is a TTY but the user has the other stream sent to a file.

So the user checks stdout is a tty, it is, keeps coloring, ignores stderr, but calls eprintln!("{}", Paint::green("bar");, is that right?

I think, in general, you want to check if both are a tty and disable if either isn't. I don't know of applications that try to do emit colors based on the specific output. What's more, it's totally possible for stdout and/or stderr to change while executing. I don't think there's anything reasonable yansi can do here.

User error is commonly a manifestation of a systemic failure that led to the user error. yansi gives users a tool that is overly broad and doesn't provide any guidance on when to use it / not use it.

I disagree on the latter part of this sentence. lib.rs contains quite a few examples on using enable() and disable(). Anything can always be misused. We cannot possible guard the user from doing "the wrong thing" or even know what "the wrong thing" is in this circumstance. Maybe they want to send colors to non-TTYs. Who knows?

print!("{} light, {} light!", Paint::green("Green").stdout(), Paint::red("red").underline().stdout());

I have no idea how this would be implemented. I don't get to know if we're writing to stdout, stderr, a network socket, a string, or anything else. What's more, the API is now quite intrusive.

I don't think there's anything wrong with Paint::enable() or Paint::disable(). If there's more guidance we can give users, I'm all for it, but otherwise I'm relatively happy with these APIs. The only thing I've wanted myself is the ability to selectively omit some outputs. For example, in Rocket, we don't want to emit emojis on Windows. It would be nice to just be able to do something like:

println!("Made with {}.", Paint::new("❤️").omit_if_else(WINDOWS, "heart");

I'm working on something that would enable this, in fact.

We currently write:

impl PaintExt for Paint<&str> {
    /// Paint::masked(), but hidden on Windows due to broken output. See #1122.
    fn emoji(_item: &str) -> Paint<&str> {
        #[cfg(windows)] { Paint::masked("") }
        #[cfg(not(windows))] { Paint::masked(_item) }
    }
}

Which works reasonably well.

[epage]

I think, in general, you want to check if both are a tty and disable if either isn't. I don't know of applications that try to do emit colors based on the specific output.

• clap distinguishes between stdout/stderr for coloring
• Maybe there is a bug in cargo but I did cargo check > l and stderr was colored.

What's more, it's totally possible for stdout and/or stderr to change while executing. I don't think there's anything reasonable yansi can do here.

When users are doing more advanced stuff, the onus is on them and they can use --coloir <when> flags. That doesn't negate doing any work to do things correctly.

Anything can always be misused. We cannot possible guard the user from doing "the wrong thing"

I opened this issue not about preventing the wrong thing but helping direct the user to the right thing. Not everyone will think through the implications of the docs on disable and will assume what is being recommended is a best practice. Some users will not read the docs, but rely on the API itself, or will miss parts of the docs. Docs are an aid but API design is where we can really help guide the user.

or even know what "the wrong thing" is in this circumstance. Maybe they want to send colors to non-TTYs. Who knows?

Yes, there are infinite possibilities of what someone might do but there are also common practices. API design should make the commonly accepted case easy while not blocking people from doing the things we haven't considered yet.