Only apply colors if stderr is a terminal

[smwoj]

As discussed in #31.

[tommilligan]

@smwoj could you investigate if there's a way to force colors for our tests, even though we are not in an interactive terminal?

It would be great to test both ways, but given only one I'd prefer keeping the existing tests unchanged.

I had to authorize the workflow a couple of times, feel free to ping me again if you want me to run CI on this branch again

[smwoj]

@tommilligan the natural way to keep the existing tests (though perhaps a little bit hacky) would be to always have colors enabled if we're running tests. (keeping the existing tests is the starting point - whether we add add more tests for the nocolor mode is a separate issue)

#[cfg(test)]
fn should_apply_color() -> bool {
    true
}

#[cfg(not(test))]
fn should_apply_color() -> bool {
    use std::io::IsTerminal;
    use std::sync::OnceLock;
    static IS_STDERR_TERMINAL: OnceLock<bool> = OnceLock::new();

    *IS_STDERR_TERMINAL.get_or_init(|| std::io::stderr().is_terminal())
}

However this doesn't work, because tests in $crate/tests directory are treated differently than regular unit tests kept inside $crate/src - they are "integration tests" that see the crate exactly as the user would. Because of this #[cfg(test)] annotations have no effect on tests stored in $crate/tests. 

You said that you'd prefer to keep the existing tests unchanged, but perhaps moving them to $crate/src and using the solution above would be OK? Technically speaking no test body would be changed.

I must admit I see the value of having this kind of integration tests (that see the crate as a client would), but I don't see a way of adding no-color fallback and keeping the tests as they are in the current CI unless we add extra configuration. However this extra configuration doesn't seem to be a good idea either:

• feature-based configuration (e.g. feature = "color_never" or feature = "color_always") is not a good fit because features should be additive,
• allowing the user to dynamically configure the value of "should_apply_color" setting is not a great fit either, because it would require running some initialization code at the beginning of every test that uses pretty_assertions. (because the default test harness in cargo has no way of running tests setup code and by default tests are run in parallel and in unpredictable order). This is clearly not a great user experience, but maybe it's good enough to use this approach in "integration" tests of this crate. 

Please let me know if one of these solutions seems good enough for you or in these circumstances we should close the PR (and perhaps leave the "nocolor" setting for a PR that introduces proper configuration across the board).

[tommilligan]

Thanks for the summary, that's a really nice writeup.

I think the most natural fit would be adding a detect-terminal feature with the changes in this PR. Then in the case not(feature = "detect-terminal") we always apply colours.

This is basically the same as your cfg(test) example but lets us configure the integration tests by not using this feature. This is still additive because it's the detection we're adding, not the color.

This also has the nice property that if it's a new feature, users can opt in to it so it's not a breaking change. We could also decide to make it a default feature in future.

[smwoj]

I think the most natural fit would be adding a detect-terminal feature with the changes in this PR. Then in the case not(feature = "detect-terminal") we always apply colours.

That's indeed a nice compromise. I pushed these changes, thanks.

It should be noted that CI is running on rust 1.63, but IsTerminal trait was added in std library 1.70. I expect the CI to pass now, but users who wish to enable it will have to use at least 1.70. I think this is OK since by default there is no breakage (the feature is disabled) and if someone opts-in detect-terminal it's fair to require more recent version.

@tommilligan could you please rerun the CI, then?