Skip to content

fix(cli): disable ANSI colors for redirected output - #38

Open
PandaHUN777 wants to merge 2 commits into
opensource-nepal:mainfrom
PandaHUN777:fix/no-color-for-redirected-output
Open

PandaHUN777 wants to merge 2 commits into
opensource-nepal:mainfrom
PandaHUN777:fix/no-color-for-redirected-output

Conversation

@PandaHUN777

Copy link
Copy Markdown

Disables ANSI colors independently for stdout and stderr when the corresponding stream is redirected, and honors a non-empty NO_COLOR environment variable. Adds regression tests for redirected output and the color selection rules, with a README note.

Closes #30

Validation:

  • cargo +stable build
  • cargo +stable test
  • cargo +stable fmt --all --check
  • cargo +stable clippy --all-targets --all-features -- -D warnings
  • git diff --check

@JustBipin

JustBipin commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Hey, this is a great PR — I really like that you checked stdout and stderr separately instead of just one global flag, and the little truth table for should_color is a nice way to pin down the empty-vs-non-empty NO_COLOR rule.

One thing I'd change though: reading std::env::var_os("NO_COLOR") from inside color() means our formatter is reaching out to the environment on every print. Neither the tty nor the env var can change halfway through a run, so there's no reason to re-check it each time. And we already have OutputConfig getting passed around everywhere for quiet/verbose — feels like color belongs in there with them.

Something more like:

// command.rs — the one place that touches the environment
let no_color = env::var_os("NO_COLOR");
let output = OutputConfig {
    color_stdout: color_enabled(io::stdout().is_terminal(), no_color.as_deref()),
    color_stderr: color_enabled(io::stderr().is_terminal(), no_color.as_deref()),
    ..OutputConfig::new(args.quiet, args.verbose)
};

// console.rs — formatter just gets told yes or no
fn color(text: &str, code: &str, enabled: bool) -> String {
    if enabled { format!("{code}{text}{RESET}") } else { text.to_owned() }
}

pub fn success(message: &str, config: &OutputConfig) {
    if config.quiet { return; }
    println!("{}", green(message, config.color_stdout));
}

Then color() is a pure function of its arguments, and console.rs doesn't even need use std::io::IsTerminal anymore.

@JustBipin

Copy link
Copy Markdown
Member

Small thing on the test side — just a heads up so you don't waste time on it.

A NO_COLOR test written the usual way wouldn't actually prove much. Command::output() captures stdout/stderr through pipes, so is_terminal() is already false and colors are off no matter what NO_COLOR is set to. It'd pass even if that branch got deleted entirely.

Your unit test on should_color is what really pins the behaviour down, so that part's solid. Nothing checks that we actually read the variable though. If the decision moves into OutputConfig (the note above), you could build one in a test and assert the field directly instead of trying to catch it through the binary:

let cfg = /* resolved with NO_COLOR=1 and a terminal */;
assert!(!cfg.color_stdout);

Disables ANSI colors independently for stdout and stderr when redirected,
and honors a non-empty NO_COLOR environment variable. Resolves output color
policy during CLI initialization in command.rs and keeps console formatting
functions pure.

Closes opensource-nepal#30
@PandaHUN777
PandaHUN777 force-pushed the fix/no-color-for-redirected-output branch from 0c1f4ef to 315be6c Compare September 29, 2026 22:32
@PandaHUN777

Copy link
Copy Markdown
Author

Thanks for the thorough review and architectural suggestions, @JustBipin!

I have refactored the color resolution to align with your design:

  • Moved terminal detection and NO_COLOR environment inspection into command.rs at CLI entry, populating color_stdout and color_stderr on OutputConfig.
  • Made color(), green(), and red() in console.rs pure formatting functions that take enabled: bool, removing IsTerminal and environment access from console.rs.
  • Placed should_color and its truth-table tests in config.rs, along with a unit test asserting that non-empty NO_COLOR disables colors on a resolved OutputConfig.
  • Maintained the redirected output regression test in tests/cli.rs without adding an ineffective subprocess NO_COLOR test.

Validated with:

  • cargo fmt --all --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test (all unit and integration tests passing)
  • git diff --check

@JustBipin

Copy link
Copy Markdown
Member

Nice — the refactor landed exactly how I hoped. Much cleaner than reading the env inside the formatter.

One last thing before merge: clippy is unhappy.

error: this boolean expression can be simplified
  --> src/config.rs:21:20
   |
21 |     is_terminal && !no_color.is_some_and(|value| !value.is_empty())
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `no_color.is_none_or(|value| value.is_empty())`

cargo clippy --all-targets --all-features -- -D warnings exits 101 on it. CI only runs build + test so it won't go red, but CONTRIBUTING.md lists clippy as a required pre-PR gate, so it's worth fixing now.

The suggestion works as-is — is_none_or has been stable since 1.82 and we're on 1.85. Same behavior, just reads better:

is_terminal && no_color.is_none_or(|value| value.is_empty())

Verified cargo fmt --all --check and cargo test are both clean on the branch, so this is the only thing left.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable colored output when stdout/stderr is not a terminal

2 participants