Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch from termcolor to anstream and anstyle #737

Merged
merged 20 commits into from
Apr 18, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ fn main() {

let Cargo::SemverChecks(args) = Cargo::parse();

configure_color(args.color_choice);
configure_color(args.color_choice.as_choice());

if args.bugreport {
use bugreport::{bugreport, collector::*, format::Markdown};
Expand Down Expand Up @@ -112,20 +112,24 @@ fn exit_on_error<T>(log_errors: bool, inner: impl Fn() -> anyhow::Result<T>) ->

/// helper function to determine whether to use colors based on the (passed) `--color` flag
/// and the value of the `CARGO_TERM_COLOR` variable.
fn configure_color(cli_choice: colorchoice_clap::Color) {
cli_choice.write_global();
// if it is still auto (i.e., user passed `--color=auto` or omitted the flag entirely),
// we check the env var. otherwise, the flag takes precedence over whatever the env var is
if ColorChoice::global() == ColorChoice::Auto {
fn configure_color(cli_choice: ColorChoice) {
// if the --color flag is explicitly set to something other than `auto`,
// this takes precedence over the environment variable, so we write that
if cli_choice != ColorChoice::Auto {
cli_choice.write_global();
} else {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't an explicitly passed --color=auto CLI flag override whatever CARGO_TERM_COLOR is set to? I find this note a bit surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, and in playing around with cargo, that's the behavior it does. The colorchoice_clap crate doesn't currently distinguish between an explicitly-passed --color=auto and no color flag entirely. I will try right now if it works with an Option<color_choice::Color> but if that doesn't play nice with #[clap(flatten)], we might have to move back to the bespoke color configuration flag that I previously had.

Relevant tangent: In doing this, I discovered that the environment variable actually takes precedence over the CLI flag, unlike what we had previously done. It's only been a little while since I added the --color flag in general; do we want to match cargo's behavior and read the environment variable over the flag?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's interesting! Yes, let's match cargo's behavior then -- and ideally, let's document this if at all possible, since I imagine other people might find this a bit unexpected as well.

If we don't document it, I'd be shocked if we don't get a "bug report" on that behavior in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, little bit of a false alarm - it looks like cargo help checks the CARGO_TERM_COLOR env var but not the --color flag - may be a tiny bug in cargo itself. When I test with CARGO_TERM_COLOR=env cargo check --color=cli, cli overrides env if it is set. However, you're right that we still need to distinguish an explicit --color=auto from one that's not set - I'll work on that.

// we match the behavior of cargo in
// https://doc.rust-lang.org/cargo/reference/config.html#termcolor
// we don't need to support `always-ansi`; that is more of an implementation detail
// and it is not recognized by cargo proper either.
// note that [`ColorChoice::AlwaysAnsi`] is not supported by cargo.
match env::var("CARGO_TERM_COLOR").as_deref() {
Ok("always") => ColorChoice::Always.write_global(),
Ok("never") => ColorChoice::Never.write_global(),
// we don't need to test for `Auto`, since it is already set
// note: the default global color value is `Auto`
Ok("auto") => ColorChoice::Auto.write_global(),
// ignore invalid or not set environment variables,
// color choice will default to `Auto`
_ => (),
}
};
}
}

Expand Down
Loading