diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f273c45..4c161d69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,8 @@ - `x$1` where the right-hand side is not an identifier +- Air no longer emits colored output when stderr is not a terminal, such as when stderr is redirected to a file. + # 0.12.0 - New `--no-configuration` flag to format with Air's built-in defaults without loading a project or user configuration file (#479, @LeonidasZhak). diff --git a/crates/air/Cargo.toml b/crates/air/Cargo.toml index 90665fd9..c053c74d 100644 --- a/crates/air/Cargo.toml +++ b/crates/air/Cargo.toml @@ -24,7 +24,7 @@ lsp = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } -tracing-subscriber = { workspace = true } +tracing-subscriber = { workspace = true, features = ["ansi"] } workspace = { workspace = true } [dev-dependencies] diff --git a/crates/air/src/color.rs b/crates/air/src/color.rs new file mode 100644 index 00000000..0f9472cd --- /dev/null +++ b/crates/air/src/color.rs @@ -0,0 +1,54 @@ +use std::ffi::OsStr; +use std::io::IsTerminal; + +/// Decides whether the CLI colors its output +/// +/// We use colored output in two places: +/// - Level labels in `tracing-subscriber`, like `ERROR` , which shows up in red +/// - Usage of the `colored` crate +/// +/// Both respect the `NO_COLOR` environment variable if we don't specify +/// anything, but to avoid potential differences between the two crates we do a +/// one time resolution of user overrides and `NO_COLOR` ourselves, then +/// forcibly set the option for both `colored` and `tracing-subscriber`. +pub(crate) fn init_color(no_color: bool) -> bool { + let color = + // User override via `--no-color` + !no_color + // User override via `NO_COLOR` env var + && !no_color_env_var_is_set() + // Only emit colors if the target is a terminal. We only emit colored + // output to stderr, so we don't need to check stdout. + && std::io::stderr().is_terminal(); + + // Apply our decision globally to `colored` + colored::control::set_override(color); + + // Return our decision so we can apply it to `tracing-subscriber` + color +} + +/// Is `NO_COLOR` set to a non-empty, non-zero value? +fn no_color_env_var_is_set() -> bool { + env_var_is_set(std::env::var_os("NO_COLOR").as_deref()) +} + +fn env_var_is_set(value: Option<&OsStr>) -> bool { + value.is_some_and(|value| !value.is_empty() && value != "0") +} + +#[cfg(test)] +mod test { + use std::ffi::OsStr; + + use crate::color::env_var_is_set; + + #[test] + fn test_env_var_is_set() { + assert!(!env_var_is_set(None)); + assert!(!env_var_is_set(Some(OsStr::new("")))); + assert!(!env_var_is_set(Some(OsStr::new("0")))); + assert!(env_var_is_set(Some(OsStr::new("1")))); + assert!(env_var_is_set(Some(OsStr::new("true")))); + } +} diff --git a/crates/air/src/lib.rs b/crates/air/src/lib.rs index cbafa08a..34fc6def 100644 --- a/crates/air/src/lib.rs +++ b/crates/air/src/lib.rs @@ -3,6 +3,7 @@ use crate::args::Command; use crate::status::ExitStatus; pub mod args; +mod color; mod commands; mod logging; pub mod status; @@ -10,10 +11,8 @@ pub mod status; pub fn run(args: Args) -> anyhow::Result { if !matches!(args.command, Command::LanguageServer(_)) { // The language server sets up its own logging - logging::init_logging( - args.global_options.log_level.unwrap_or_default(), - args.global_options.no_color, - ); + let color = color::init_color(args.global_options.no_color); + logging::init_logging(args.global_options.log_level.unwrap_or_default(), color); } match args.command { diff --git a/crates/air/src/logging.rs b/crates/air/src/logging.rs index 4156fc11..d4eb59bf 100644 --- a/crates/air/src/logging.rs +++ b/crates/air/src/logging.rs @@ -1,11 +1,10 @@ use std::fmt::Display; use std::str::FromStr; use tracing_subscriber::Layer; -use tracing_subscriber::Registry; use tracing_subscriber::filter; use tracing_subscriber::layer::SubscriberExt; -pub(crate) fn init_logging(log_level: LogLevel, no_color: bool) { +pub(crate) fn init_logging(log_level: LogLevel, color: bool) { let log_level = log_level.tracing_level(); // Apply the log level to each air crate. @@ -15,13 +14,9 @@ pub(crate) fn init_logging(log_level: LogLevel, no_color: bool) { filter = filter.with_target(*target, log_level); } - let mut layer = tracing_subscriber::fmt::layer(); - - if no_color { - layer = turn_off_colors(layer); - }; - - let layer = layer + let layer = tracing_subscriber::fmt::layer() + // Apply coloring decision + .with_ansi(color) // i.e. Displaying `ERROR` or `WARN` .with_level(true) // Don't show the module name, not useful in a cli @@ -40,24 +35,6 @@ pub(crate) fn init_logging(log_level: LogLevel, no_color: bool) { tracing::trace!("Initialized logging"); } -/// Explicitly turn off ANSI colored / styled output -/// -/// We use colored output in two places: -/// - Level labels in tracing-subscriber, like `ERROR` , which shows up in red -/// - Usage of the `colored` crate -/// -/// Both respect the `NO_COLOR` environment variable if we don't specify anything. -/// -/// We explicitly call `set_override()` and `with_ansi()` ONLY when turning colors off. -/// `set_override(true)` and `with_ansi(true)` override the `NO_COLOR` environment -/// variable, and we don't want to do that. -fn turn_off_colors( - layer: tracing_subscriber::fmt::Layer, -) -> tracing_subscriber::fmt::Layer { - colored::control::set_override(false); - layer.with_ansi(false) -} - #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, clap::ValueEnum)] pub(crate) enum LogLevel { Error, diff --git a/crates/air/tests/integration/format.rs b/crates/air/tests/integration/format.rs index ac652987..34e1e5aa 100644 --- a/crates/air/tests/integration/format.rs +++ b/crates/air/tests/integration/format.rs @@ -347,6 +347,32 @@ fn test_check_output_format() { ); } +#[test] +fn test_no_color_when_stderr_is_not_a_terminal() -> anyhow::Result<()> { + let directory = TempDir::new()?; + let directory = directory.path(); + + std::fs::write(directory.join("needs-formatting.R"), "1+1\n")?; + std::fs::write(directory.join("parse-error.R"), "1+/1\n")?; + + // `run()` captures stderr through pipes so `is_terminal()` is `false`, just + // like a user doing `air format R/ 2>errors.log` to pipe stderr to a log + // file + let output = Command::new(binary_path()) + .current_dir(directory) + // Defensive to be reproducible on all systems, just in case + .env_remove("NO_COLOR") + .arg("format") + .arg(".") + .arg("--check") + .run(); + + assert!(!output.stderr.contains('\x1b')); + insta::assert_snapshot!(output); + + Ok(()) +} + #[test] fn test_stdin_cant_supply_paths() -> anyhow::Result<()> { let directory = TempDir::new()?; diff --git a/crates/air/tests/integration/snapshots/integration__format__no_color_when_stderr_is_not_a_terminal.snap b/crates/air/tests/integration/snapshots/integration__format__no_color_when_stderr_is_not_a_terminal.snap new file mode 100644 index 00000000..5c5072c1 --- /dev/null +++ b/crates/air/tests/integration/snapshots/integration__format__no_color_when_stderr_is_not_a_terminal.snap @@ -0,0 +1,14 @@ +--- +source: crates/air/tests/integration/format.rs +expression: output +--- +success: false +exit_code: 255 +----- stdout ----- + +----- stderr ----- +ERROR Failed to format parse-error.R: Failed to parse due to syntax errors. +Would reformat: needs-formatting.R + +----- args ----- +format . --check