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

Conversation

suaviloquence
Copy link
Contributor

This is a draft PR to solicit feedback on the direction I'm going, what crates to use, and how we want the architecture to look like.

One of the biggest changes in moving to anstream is that stdout and stderr are global, so we don't need to store them in GlobalConfig; in fact, the library part of the crate doesn't need to be aware of the user's color choice, it just needs to use the anstream::print! and related macros.

Question: Is this a positive change for the crate? In the binary version, we configure our own color preference with anstream::ColorChoice::write_global, and another crate that uses the semver-checks library would do the same.

There's also the question of how to style the colors. Right now, I'm using the basic anstyle crate, but a lot of people seem to be using owo-colors. These are both runtime-styling, and we could also opt for a compile-time styler with color-print or maybe even anstyle-termcolors (but I haven't checked out that last one too much yet).

This also changes our environment variable interface a little. Instead of using the CARGO_TERM_COLOR variable as we were previously, anstream automatically sets colors based on the CCOLOR, CCOLOR_FORCE and NOCOLOR environment variables in the anstyle-query crate.

Question: Do we want to also support the CARGO_TERM_COLOR variable, or is this enough?

I haven't written a lot of new tests or documentation yet - I want to see how we feel about these changes.

I'm super open to feedback and changing direction at this point - I want to get input in this PR.

@suaviloquence
Copy link
Contributor Author

Linking #724

@@ -157,39 +158,35 @@ pub(super) fn run_check_release(
let mut results_with_errors = vec![];
for (semver_query, time_to_decide, results) in all_results {
config
.log_verbose(|config| {
.log_verbose(|_| {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the callbacks will need config anymore, so we could (1) make the methods log_* take only &self if we go with this architecture, (2) move the shell_print_* methods to (associated) functions, and/or (3) remove the config parameter from the closure argument

Copy link
Owner

Choose a reason for hiding this comment

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

It might be a good idea to let the config variable carry explicit Box<dyn std::io::Write> fields for stdout and stderr, and pass them here.

Imagine a user is using cargo-semver-checks as a library. It's a bit strange if a library is printing to it's parent program's stdout/stderr, right? It would be nice if the parent program could configure where and how it wants the output printed, and then decide by itself whether to actually put it on stdout/stderr.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little bit lost. When someone uses cargo-semver-checks as a library, they won't configure a separate stdout/stderr for logging, right? Do you mean running a child process?

@epage IIRC, anstyle takes care of parallel printing, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imagine a user is using cargo-semver-checks as a library. It's a bit strange if a library is printing to it's parent program's stdout/stderr, right? It would be nice if the parent program could configure where and how it wants the output printed, and then decide by itself whether to actually put it on stdout/stderr.

On main branch, I don't think there's a mechanism to set the output streams programmatically. Is that something we want? I can add it in this PR, or a different one.

Copy link
Owner

Choose a reason for hiding this comment

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

they won't configure a separate stdout/stderr for logging

Not configure separate stdout/stderr, just give them the option to redirect the cargo-semver-checks-as-library output to a buffer (or drop it altogether) instead of having it printed to their app's stdout/stderr.

I can add it in this PR, or a different one.

Not worth adding here, let's optimize this PR for being easy to review and merge. We can add that in the future, I just wanted to not remove that option from us. If this PR can make sure to print to the configured Box<dyn Write> values (or whatever other representation is best), we can handle the rest later.

src/main.rs Outdated
/// auto (based on whether output is a tty), and never
///
/// Default is auto (use colors if output is a TTY, otherwise don't use colors)
#[arg(value_enum, long = "color")]
color_choice: Option<termcolor::ColorChoice>,
color_choice: Option<ColorChoice>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, this breaks the tests now because the --color arg is now incompatible with the check-release subcommand, which is a little weird because it's basically the same enum, and I didn't change anything in the CheckRelease struct besides that. I can sort out the issue later, but if something jumps out at you for why this doesn't work suddenly, I'd love to know.

Copy link
Owner

Choose a reason for hiding this comment

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

I have very little experience with the clap crate, so I don't see anything obvious why this might be broken and I'm not sure I'd be much help on this.

Perhaps @pksunkara might have an idea though!

Copy link
Contributor

Choose a reason for hiding this comment

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

To me, it looks like the test failures are related to removal of CARGO_TERM_COLOR logic unless I am mistaken.

cmd.arg("--color=always");
}

// TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used to only add the --color=always flag if stderr was a tty, but we can respect the user's color choice more now if we want. I can also easily revert to the earlier behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since cargo uses anstyle too, I think you can just forward the color value instead of the match here. @epage WDYT?

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

I'm not that familiar with anstream, anstyle, or clap, so I imagine @pksunkara's feedback will be more valuable here -- sorry!

Comment on lines -29 to -35
let color_choice = match std::env::var("CARGO_TERM_COLOR").as_deref() {
Ok("always") => Some(ColorChoice::Always),
Ok("alwaysansi") => Some(ColorChoice::AlwaysAnsi),
Ok("auto") => Some(ColorChoice::Auto),
Ok("never") => Some(ColorChoice::Never),
Ok(_) | Err(..) => None,
};
Copy link
Owner

Choose a reason for hiding this comment

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

cargo itself uses CARGO_TERM_COLOR and the --color flag: https://doc.rust-lang.org/cargo/reference/config.html#termcolor

As a cargo extension with plans to merge into cargo one day (#61), it would massively simplify things if we did the same thing. So I'd prefer to keep this mechanism available. (If there's no alwaysansi option in the new crate, that can go since cargo doesn't use it either.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would ask @epage what he thinks about the CARGO_TERM_COLOR support since he works on both cargo and anstyle.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd be a bit surprised if anstyle got first-party support for a cargo-specific way to configure color, but perhaps there's going to be an anstyle-cargo crate :) But I'm sure epage is super busy, and I feel reasonably confident it's safe to keep CARGO_TERM_COLOR support here unless he chimes in to say otherwise.

src/main.rs Outdated
/// auto (based on whether output is a tty), and never
///
/// Default is auto (use colors if output is a TTY, otherwise don't use colors)
#[arg(value_enum, long = "color")]
color_choice: Option<termcolor::ColorChoice>,
color_choice: Option<ColorChoice>,
Copy link
Owner

Choose a reason for hiding this comment

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

I have very little experience with the clap crate, so I don't see anything obvious why this might be broken and I'm not sure I'd be much help on this.

Perhaps @pksunkara might have an idea though!

Comment on lines -288 to -305
/// Set the termcolor [color choice](termcolor::ColorChoice).
/// If `None`, the use of colors will be determined automatically by
/// the `CARGO_TERM_COLOR` env var and tty type of output.
pub fn with_color_choice(&mut self, choice: Option<termcolor::ColorChoice>) -> &mut Self {
self.color_choice = choice;
self
}

/// Get the current color choice. If `None`, the use of colors is determined
/// by the `CARGO_TERM_COLOR` env var and whether the output is a tty
#[inline]
pub fn color_choice(&self) -> Option<&termcolor::ColorChoice> {
self.color_choice.as_ref()
}
Copy link
Owner

Choose a reason for hiding this comment

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

Any particular reason why we aren't letting library users set the color choice programmatically anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

We give more preference to the color setting when someone runs a program. Even if someone uses this library, they would have their own color setting mechanism in their binary using anstyle and this library would automatically respect it. Therefore, we don't need these settings anymore in the library.

Copy link
Owner

Choose a reason for hiding this comment

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

What if that third-party program doesn't use anstyle for its own styling needs, though? Surely the answer can't be "use anstyle or you can't set cargo-semver-checks color options" right?

Copy link
Contributor

@pksunkara pksunkara Apr 10, 2024

Choose a reason for hiding this comment

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

use anstyle or you can't set cargo-semver-checks color options

I might be completely off here, but I think that's intended and the main reason for anstyle repo to exist. They can use ColorChoice::write_global to control the colors in every library they use and do not have to call each individual library's with_color_choice().

And they won't be adding a new dependency, because they already depend on cargo-semver-checks which in turn depends on colorchoice anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

You are making a solid case for using anstyle. If our downstream user is already directly using anstyle, the ColorChoice::write_global() approach is what we want them to be able to use.

But I don't see the case for removing the ability to set colors via with_color_choice(). If our downstream user is not already directly depending on anstyle, it feels wrong to tell them "if you want to control color output in cargo-semver-checks then you better add a dependency and configure it there." Even if it isn't a new dependency in the sense of "was it already being compiled," it's still one more thing they have to maintain and keep in sync.

As an example of the burden of needing such a dependency, thus far I've had to deal with more than one case of major version mismatches between a dependency A's dependency B, and our directly-added version of B that we solely added so we could properly interact with A. Let's not impose the same maintenance burden on our users, and let's offer a smooth path for configuring colors even if they don't already use anstyle.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. I didn't think of it like that. My bad.

Copy link
Owner

Choose a reason for hiding this comment

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

No worries at all! It was a good discussion and cargo-semver-checks is better off for it.

@@ -157,39 +158,35 @@ pub(super) fn run_check_release(
let mut results_with_errors = vec![];
for (semver_query, time_to_decide, results) in all_results {
config
.log_verbose(|config| {
.log_verbose(|_| {
Copy link
Owner

Choose a reason for hiding this comment

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

It might be a good idea to let the config variable carry explicit Box<dyn std::io::Write> fields for stdout and stderr, and pass them here.

Imagine a user is using cargo-semver-checks as a library. It's a bit strange if a library is printing to it's parent program's stdout/stderr, right? It would be nice if the parent program could configure where and how it wants the output printed, and then decide by itself whether to actually put it on stdout/stderr.

Comment on lines -29 to -35
let color_choice = match std::env::var("CARGO_TERM_COLOR").as_deref() {
Ok("always") => Some(ColorChoice::Always),
Ok("alwaysansi") => Some(ColorChoice::AlwaysAnsi),
Ok("auto") => Some(ColorChoice::Auto),
Ok("never") => Some(ColorChoice::Never),
Ok(_) | Err(..) => None,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I would ask @epage what he thinks about the CARGO_TERM_COLOR support since he works on both cargo and anstyle.

Comment on lines -288 to -305
/// Set the termcolor [color choice](termcolor::ColorChoice).
/// If `None`, the use of colors will be determined automatically by
/// the `CARGO_TERM_COLOR` env var and tty type of output.
pub fn with_color_choice(&mut self, choice: Option<termcolor::ColorChoice>) -> &mut Self {
self.color_choice = choice;
self
}

/// Get the current color choice. If `None`, the use of colors is determined
/// by the `CARGO_TERM_COLOR` env var and whether the output is a tty
#[inline]
pub fn color_choice(&self) -> Option<&termcolor::ColorChoice> {
self.color_choice.as_ref()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We give more preference to the color setting when someone runs a program. Even if someone uses this library, they would have their own color setting mechanism in their binary using anstyle and this library would automatically respect it. Therefore, we don't need these settings anymore in the library.

src/main.rs Outdated
@@ -112,6 +111,29 @@ fn exit_on_error<T>(log_errors: bool, inner: impl Fn() -> anyhow::Result<T>) ->
}
}

/// helper enum to derive [`clap::ValueEnum`] on [`anstream::ColorChoice`]
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub(crate) struct ColorChoice(pub(crate) anstream::ColorChoice);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would recommend using https://github.com/rust-cli/anstyle/tree/main/crates/colorchoice-clap instead of writing your own.

src/main.rs Outdated
Comment on lines 15 to 17
if let Some(color_choice) = args.check_release.color_choice {
color_choice.0.write_global();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If using colorchoice-clap, this can be changed to something like:

Suggested change
if let Some(color_choice) = args.check_release.color_choice {
color_choice.0.write_global();
}
args.check_release.color_choice.write_global();

cmd.arg("--color=always");
}

// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Since cargo uses anstyle too, I think you can just forward the color value instead of the match here. @epage WDYT?

@@ -157,39 +158,35 @@ pub(super) fn run_check_release(
let mut results_with_errors = vec![];
for (semver_query, time_to_decide, results) in all_results {
config
.log_verbose(|config| {
.log_verbose(|_| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little bit lost. When someone uses cargo-semver-checks as a library, they won't configure a separate stdout/stderr for logging, right? Do you mean running a child process?

@epage IIRC, anstyle takes care of parallel printing, right?

src/main.rs Outdated
/// auto (based on whether output is a tty), and never
///
/// Default is auto (use colors if output is a TTY, otherwise don't use colors)
#[arg(value_enum, long = "color")]
color_choice: Option<termcolor::ColorChoice>,
color_choice: Option<ColorChoice>,
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, it looks like the test failures are related to removal of CARGO_TERM_COLOR logic unless I am mistaken.

it still breaks tests, i think it's an issue with how we handle the
`check-release` subcommand
src/main.rs Outdated
@@ -291,12 +288,12 @@ struct CheckRelease {
verbosity: clap_verbosity_flag::Verbosity<clap_verbosity_flag::InfoLevel>,

/// Whether to print colors to the terminal:
/// always, alwaysansi (always use only ANSI color codes),
/// always, always-ansi (always use only ANSI color codes),
/// auto (based on whether output is a tty), and never
///
/// Default is auto (use colors if output is a TTY, otherwise don't use colors)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove the docstring on this field.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, why remove it? Will clap automatically display the --color options in --help if we do that?

The --color options are non-obvious and I think it's important to have a help string in the CLI for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

The colorchoice_clap has a docstring for the color field that gets picked up by clap when used with flatten. It looks something like the following:

      --color <WHEN>  Controls when to use color [default: auto] [possible values: auto, always, never]

Copy link
Owner

Choose a reason for hiding this comment

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

Ah neat! This wasn't obvious to me from the colorchoice_clap docs, so perhaps best to leave a regular (non-doc) comment next to the field indicating we expect the built-in --help text there.

@suaviloquence
Copy link
Contributor Author

re @pksunkara: I switched to the colorchoice-clap crate and the test with the --color=always flag is still not passing. I think it's because we store two parallel sets of config in args.check_release and one in the args.command which is an Option<subcommand> where the only existing subcommand just stores another CheckRelease struct. So when we are calling cargo semver-checks check-release ... only one of the options is set. @obi1kenobi: do we need 2 parallel versions of the same config, or does it make more sense to refactor to keep the subcommand but only store one version of the config? (or we could move check-release to be a flag? I need to double-check what it does)

@pksunkara
Copy link
Contributor

I would recommend moving the color flag entirely to SemverChecks struct. I am not sure what @obi1kenobi thinks.

src/main.rs Outdated
@@ -291,12 +288,12 @@ struct CheckRelease {
verbosity: clap_verbosity_flag::Verbosity<clap_verbosity_flag::InfoLevel>,

/// Whether to print colors to the terminal:
/// always, alwaysansi (always use only ANSI color codes),
/// always, always-ansi (always use only ANSI color codes),
/// auto (based on whether output is a tty), and never
///
/// Default is auto (use colors if output is a TTY, otherwise don't use colors)
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, why remove it? Will clap automatically display the --color options in --help if we do that?

The --color options are non-obvious and I think it's important to have a help string in the CLI for them.

src/main.rs Outdated
Comment on lines 290 to 296
/// Whether to print colors to the terminal:
/// always, alwaysansi (always use only ANSI color codes),
/// always, always-ansi (always use only ANSI color codes),
/// auto (based on whether output is a tty), and never
///
/// Default is auto (use colors if output is a TTY, otherwise don't use colors)
#[arg(value_enum, long = "color")]
color_choice: Option<termcolor::ColorChoice>,
#[command(flatten)]
color_choice: colorchoice_clap::Color,
Copy link
Owner

Choose a reason for hiding this comment

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

I might be confused here, but colorchoice_clap::Color seems like it might not have alwaysansi as an option, whereas anstream::ColorChoice does. Do we want to use anstream::ColorChoice here?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, AlwaysAnsi is skipped in clap because Always always resolves to it except when running on a Windows console. And when someone's using a Windows console, they wouldn't want to use AlwaysAnsi option anyway.

Code for reference: https://github.com/rust-cli/anstyle/blob/ed1a598cc2c3b84483c6058a982d045f7f7aecfc/crates/anstream/src/auto.rs#L110-L126

src/main.rs Outdated
@@ -291,12 +288,12 @@ struct CheckRelease {
verbosity: clap_verbosity_flag::Verbosity<clap_verbosity_flag::InfoLevel>,

/// Whether to print colors to the terminal:
/// always, alwaysansi (always use only ANSI color codes),
/// always, always-ansi (always use only ANSI color codes),
Copy link
Owner

Choose a reason for hiding this comment

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

Make sure that the string shown here is an exact match for what the CLI accepts. So if we're making this change, --color=always-ansi must work. I believe that previously, --color=alwaysansi was required and always-ansi was not accepted.

@suaviloquence
Copy link
Contributor Author

This commit moves stdout and stderr into GlobalConfig to set them programmatically if wanted by the library. I think we still prefer setting color choice in ColorChoice::write_global, but if the user wants cargo-semver-checks to have different color preferences than other crates, we can do that here.

src/config.rs Outdated
/// in [`ColorChoice::write_global`] if you are using the `anstream` crate.
///
/// See also [`GlobalConfig::set_out_color_choice`] and [`GlobalConfig::set_color_choice`]
pub fn set_err_color_choice(&mut self, choice: ColorChoice) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pksunkara unless I'm missing something, there seems to be no way to change a specific stream's color choice after initialization. If this is true, we can either do what I did to set the colors, which is a little hacky, or we can take an owned self and call the method like with_{out,err,}color_choice.

src/config.rs Outdated
Comment on lines 179 to 182
/// Sets the stderr output stream
///
/// Defaults to the global color choice setting in [`ColorChoice::global`].
/// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Sets the stderr output stream
///
/// Defaults to the global color choice setting in [`ColorChoice::global`].
/// Call [`GlobalConfig::set_err_color_choice`] to customize the color choice
/// Sets the stdout output stream
///
/// Defaults to the global color choice setting in [`ColorChoice::global`].
/// Call [`GlobalConfig::set_out_color_choice`] to customize the color choice

a bit of copy-pasta here

Squashed commit of the following:

commit 59a3a7f
Author: m <[email protected]>
Date:   Fri Apr 12 15:15:42 2024 -0700

    add some simple tests

commit 03b2494
Author: m <[email protected]>
Date:   Fri Apr 12 14:16:55 2024 -0700

    current work
@suaviloquence
Copy link
Contributor Author

Added some simple tests. Some things to consider:

  • Do we want our set_color methods to just take a bool? It doesn't make a lot of sense for a user to call set_color(Auto) when that is already the default.
  • What should the precedence of ColorChoice::write_global vs GlobalConfig::set_color be? I'm thinking set_color over write_global but that falls apart a little when we do GlobalConfig::set_color(Auto) which is one of the reasons for wanting to configure it w a boolean instead.

@suaviloquence
Copy link
Contributor Author

also @obi1kenobi: do we want to read the CARGO_TERM_COLOR on the main/binary side or on the library/GlobalConfig side?

@obi1kenobi
Copy link
Owner

Excellent highly-nuanced questions. I love the care with which you're approaching this!

  • Do we want our set_color methods to just take a bool? It doesn't make a lot of sense for a user to call set_color(Auto) when that is already the default.

  • What should the precedence of ColorChoice::write_global vs GlobalConfig::set_color be? I'm thinking set_color over write_global but that falls apart a little when we do GlobalConfig::set_color(Auto) which is one of the reasons for wanting to configure it w a boolean instead.

You've clearly thought about this a fair bit. What do you think we should do?

do we want to read the CARGO_TERM_COLOR on the main/binary side or on the library/GlobalConfig side?

I'm inclined to say on the binary side, not the library side. What do you think?

My reasoning is that when we're running as a binary, we're a cargo extension. When we're running as a library, we're a part of some other program that has its own assumptions and responsibilities, and which may be surprised if cargo's assumptions are imposed on it.

- document that `ColorChoice::write_global` must be called before
  `GlobalConfig::new` or `set_std{err,out}`
- add tests for `GlobalConfig`
- add parsing for `CARGO_TERM_COLOR` env var in binary part of crate
@suaviloquence
Copy link
Contributor Author

Awesome! I think it makes a lot of sense to use bools to set color. Also, I realized that, unlike anstream::std{out,err}, we only read the ColorChoice::global value on (re-)initialization of the streams in GlobalConfig::new and set_std{out,err}. I don't think this is necessarily a problem; just something we have to note in the tests so the user knows to call ColorChoice::write_global before creating the GlobalConfig instance if they want to configure color that way. I added a test to make sure that this behavior is working as intended.

I also agree that it makes more sense to read the env var in the binary part of the crate, so I implemented that, and now the tests pass (though it seems like with rustdoc json v29, I still won't get passing CI 🙃 (this time though, it's not the fault of the PR)).

Thanks for all your help, and let me know if you want something to change!

Copy link
Contributor

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Everything LGTM

@obi1kenobi
Copy link
Owner

The nightly step is blocked pending the release of a new version of an upstream crate that I'm not a maintainer of, but it's a non-required build step so we can merge without it being green.

I'll go over the code one more time now -- thank you both for all the effort you've put in here!

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks in pretty good shape, just some polish items left. Thanks for putting this together! I'm excited to merge it.

The change in how library consumers might set color options is relatively significant, and deserves to be noted in the release notes. Would you mind helping me out and writing up a small migration guide as a comment in this PR before we merge it? All it needs to show is a couple of code examples along the lines of "if you did X in the previous version, in the new version you'll need to do Y instead."

use std::{collections::BTreeMap, sync::Arc, time::Instant};

use anstyle::{AnsiColor, Color, Reset, Style};

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 387 to 414
pub fn check_release(&self) -> anyhow::Result<Report> {
let mut config = GlobalConfig::new().set_level(self.log_level);
// we don't want to set auto if the choice is not set because it would overwrite
// the value if the CARGO_TERM_COLOR env var read in GlobalConfig::new() if it is set
if let Some(choice) = self.color_choice {
config = config.set_color_choice(choice);
}
Copy link
Owner

Choose a reason for hiding this comment

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

With the color-setting functionality now on GlobalConfig instead of on this type, how would a user of the library go about configuring color settings?

Unless I'm missing something, it seems that the GlobalConfig is created and then immediately used here without any opportunity for the consumer of the library to use its color-setting methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of a big (and semver-breaking) change, but would you be opposed to taking a GlobalConfig (or a &mut GlobalConfig) as a parameter to the check_release function? Otherwise, it's possible, but we would need to double our configuration API - have one call for Check and one call for GlobalConfig.

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, we can do that. Do you think taking it by value or by &mut is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it take &mut to leave the option open for a library user to run multiple Checks

src/main.rs Outdated
Comment on lines 116 to 118
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
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a note here about what the expected value of cli_choice is if the flag was not present in the CLI invocation?

This unconditional cli_choice.write_global() call is tripping me up here a bit since I'm unfamiliar with the underlying API's assumptions. It's usually a good idea to leave an explanatory note for our future selves (or for other future maintainers) rather than relying on everyone to read and recall upstream libraries' docs each time.

Comment on lines 142 to 144
// if config.is_stderr_tty() {
// cmd.arg("--color=always");
// }
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// if config.is_stderr_tty() {
// cmd.arg("--color=always");
// }

Copy link
Contributor Author

@suaviloquence suaviloquence Apr 14, 2024

Choose a reason for hiding this comment

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

I updated this so it reads the current stderr configuration. Let me know if the documentation isn't clear or if this is too "hacky". Right now, we don't have a way to read the current color choice in our config API, but I can add that if we want it. However, because we write the child process stderr (which has colors) to our AutoStream that strips colors if we indicate our preference for that, we will get the correct color preferences here. I can add a test here for if you think that's a good idea.

this commit

Copy link
Owner

Choose a reason for hiding this comment

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

If you don't mind writing a test, I'd love to have one! AutoStream's behavior is nice but a bit surprising, so it'd be nice to have a test we can point future readers of this code to (including me).

It would be especially useful if in the future we yet again switch libraries, and the next library's stream implementation doesn't do this kind of magically-automatic ANSI code stripping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little more nebulous than I thought - it works when RustdocCommand::silence is true, but when it is false, since we just forward the rustdoc err stream to stderr (actual tty stderr, not GlobalConfig::stderr), we're not currently using the configured stderr for the rustdoc progress stream. We can redirect it using Command::spawn and a thread that acts as a pipe between the child process stderr and our stderr if we want. Do you think a library user would expect this behavior? That is, would they expect the rustdoc progress output to go through the cargo-semver-checks' configured stderr. I'm leaning towards yes, but it will add more complexity to the function because we have to use threads.

By the way, thanks for all your patience and great feedback on this PR!

Copy link
Owner

Choose a reason for hiding this comment

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

I think definitely not in this PR, and possibly not at all. We'll have to solicit feedback from users, and I'm wary of taking on even more technical complexity when we aren't sure users want the feature it produces.

Maybe open an issue describing the concern and the nature of the decision point here, and we revisit it in the future. Otherwise this PR for switching to a different stream coloring library is going to end up being a giant revamp of everything, and it'll take forever to merge.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, thank you for all the care and skill you are investing into cargo-semver-checks! It's my pleasure to help as best I can, and it's lovely to work with someone as thoughtful and kind as you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, that makes a lot of sense. I just re-added the color flag on our rustdoc invocation to respect the color preferences. Do you still want a test for this? It's not using any of the anstream magic anymore.

Copy link
Owner

Choose a reason for hiding this comment

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

If we aren't materially changing the code here, it's fine as it was without a test.

But if you have a clever idea for how to elegantly write a test for something that previously wasn't tested, I wouldn't say no!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the generate_rustdoc function is too long and does too much to be able to effectively test something like this. It might be worth breaking it up in the future so we can write tests that make these behaviors more visible and guaranteed. Since the behavior stays the same here, I don't know if it's worth writing the test in this already-big PR, but I'd be open to refactoring and writing it in the future.

@obi1kenobi obi1kenobi added the C-enhancement Category: raise the bar on expectations label Apr 14, 2024
src/main.rs Outdated
Comment on lines 116 to 120
// 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 had to make our own colorchoice clap handler
because the mixin doesn't work with Option<_>
@suaviloquence
Copy link
Contributor Author

suaviloquence commented Apr 18, 2024

Migration Guide

Major changes

  • Check::check_release takes a new &mut GlobalConfig parameter, and GlobalConfig now holds log level and color settings.
    • Change:
      let mut check = Check::new(rustdoc);
      check.with_packages(packages)
           .with_log_level(Some(log_level))
           .with_color_choice(Some(termcolor::ColorChoice::Always));
      let result = check.check_release();
    • To:
      let mut config = GlobalConfig::new()
         .set_level(Some(log_level));
      config.set_color_choice(true); // or set_{err,out)_color_choice for finer-grained control
      
      let mut check = Check::new(rustdoc);
      check.with_packages(packages);
      let result = check.check_release(&mut config);
  • Set color choice in one of three ways:
    • Change:
      let choice: termcolor::ColorChoice = ...;
      config = config.set_color_choice(choice);
    • To: one of the three following options, depending on desired granularity
      1. Set global color choice for all crates using the anstream library:
        let choice: anstream::ColorChoice = ...;
        // set *before* creating `GlobalConfig` instance (if using default stdout/stderr)
        // or before calling `GlobalConfig::set_std{out,err}` (if configuring them)
        choice.write_global(); // configures `choice` for all crates using `anstream`
      2. Set whether to use colors for both stdout and stderr in the cargo-semver-checks library:
        let use_colors: bool = ...;
        config.set_color_choice(use_colors);
      3. Set whether to use colors for a specific stream in the cargo-semver-checks library:
        let use_colors_for_stderr: bool = ...;
        let use_colors_for_stdout: bool = ...;
        config.set_err_color_choice(use_colors_for_stderr);
        config.set_out_color_choice(use_colors_for_stdout);

New features

  • Users can configure the stdout and stderr streams for cargo-semver-checks to be different from default:
    let stderr: Box<dyn std::io::Write> = Box::new(stderr_buffer);
    config.set_stderr(stderr);

Minor changes

  • #[must_use] added on GlobalConfig::std{err,out}
  • GlobalConfig::shell_print takes an anstyle::Color instead of a termcolor::Color:
    • Change config.shell_print(status, message, termcolor::Color::Red, justified)
    • To config.shell_print(status, message, anstyle::Color::Ansi(anstyle::AnsiColor::Red, justified)
  • To write colors to stdout/stderr, use writeln!(config.stdout(), "{}styled text", anstyle::Style::new().bold().[...]); instead of the termcolor API
  • use GlobalConfig::{out,err}_color_choice() to query whether to print colors for the stdout/stderr stream instead of using GlobalConfig::is_err_tty as a heuristic
  • GlobalConfig::std{out,err} returns an opaque impl Write + '_ type instead of a termcolor::StandardStream

Does this look good? Did I miss anything?

this also cleans up lines that get unnecessarily changed in the diff
@suaviloquence suaviloquence marked this pull request as ready for review April 18, 2024 18:20
@suaviloquence suaviloquence changed the title [draft] switch from termcolor to anstream and anstyle switch from termcolor to anstream and anstyle Apr 18, 2024
@obi1kenobi
Copy link
Owner

Looks great! I tweaked the Rust code formatting in the release notes a bit, to make it a bit easier to read without scrolling.

Question on method naming: it seems like we now have a set_level() method where before we had a with_log_level() method. Is this divergence intentional, or should we instead have either set_log_level() or with_log_level() as before? Similar question about with the other set_...() methods, as opposed to the existing with_...() methods' naming convention.

@suaviloquence
Copy link
Contributor Author

Question on method naming: it seems like we now have a set_level() method where before we had a with_log_level() method. Is this divergence intentional, or should we instead have either set_log_level() or with_log_level() as before? Similar question about with the other set_...() methods, as opposed to the existing with_...() methods' naming convention.

The GlobalConfig::set_level method already existed before this PR, so I left it unchanged. Additionally, since we used to have that and the set_color_choice method, I used the set_ prefix for GlobalConfig setters. I agree that it's not consistent between GlobalConfig and Check, but they are both internally consistent. Should we use the same prefix for everything? If so, in here, or in a different PR?

@obi1kenobi
Copy link
Owner

Ah bummer, it must have snuck in.

If you're open to fixing it up, separate PR would be best. Let's merge this one :)

@obi1kenobi obi1kenobi enabled auto-merge (squash) April 18, 2024 23:14
@obi1kenobi obi1kenobi merged commit aa44576 into obi1kenobi:main Apr 18, 2024
30 of 31 checks passed
@obi1kenobi
Copy link
Owner

Thanks for all the work you did to make this happen! It wasn't the easiest PR to merge, and to be honest I was dreading doing the work a bit 😅 I really appreciate all the care and attention you put into making this happen -- it was very helpful!

@suaviloquence
Copy link
Contributor Author

Thanks for all the work you did to make this happen! It wasn't the easiest PR to merge, and to be honest I was dreading doing the work a bit 😅 I really appreciate all the care and attention you put into making this happen -- it was very helpful!

Of course, and thanks again for all your help. Glad to finally have a finished version of it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants