fix(cli): logging color #7441
fix(cli): logging color #7441
Conversation
🦋 Changeset detectedLatest commit: b938b6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds colour handling to CLI logging. Introduces ColorsArg helpers (is_enabled/is_disabled). Propagates an optional colour argument from command execution to logging initialisation by extending setup_cli_subscriber’s signature. Logging now toggles ANSI based on ColorsArg: enabled when None or Force; disabled when Off. Includes a changeset referencing #3824. Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_cli/src/logging.rs (1)
24-31: Defaulting to ANSI whencolorsisNonebreaks piped output. Autodetect TTY and disable for files.This currently enables colours even when output is piped or redirected, contradicting #3824. Compute ANSI as:
Force→ on.Off→ off.None→ on only if stdout is a TTY; if writing to a file, off.Apply:
- let mut format = tracing_subscriber::fmt::layer() + let use_ansi = match (file.is_some(), colors.map(|c| c.is_enabled())) { + // Explicit user choice wins + (_, Some(true)) => true, + (_, Some(false)) => false, + // No explicit choice: disable for files, autodetect for stdout + (true, None) => false, + (false, None) => std::io::stdout().is_terminal(), + }; + + let mut format = tracing_subscriber::fmt::layer() .with_level(true) .with_target(false) .with_thread_names(true) .with_file(true) - .with_ansi(colors.is_none_or(|c| c.is_enabled())); + .with_ansi(use_ansi);Add (outside this hunk) the import for detection:
use std::io::IsTerminal as _;Also, to keep MSRV flexible, prefer
map_or/is_some_andover neweris_none_or.crates/biome_cli/src/commands/mod.rs (1)
605-622: Borrowed temporaries: returningOption<&ColorsArg>risks dangling refs. Return by value.
Some(&ColorsArg::Off/Force)creates references to temporaries. ReturnOption<ColorsArg>and clone the existing flag when present.- pub const fn get_color(&self) -> Option<&ColorsArg> { + pub fn get_color(&self) -> Option<ColorsArg> { match self.cli_options() { Some(cli_options) => { // To properly display GitHub annotations we need to disable colors - if matches!(cli_options.reporter, CliReporter::GitHub) { - return Some(&ColorsArg::Off); + if matches!(cli_options.reporter, CliReporter::GitHub) { + return Some(ColorsArg::Off); } // We want force colors in CI, to give e better UX experience // Unless users explicitly set the colors flag - if matches!(self, Self::Ci { .. }) && cli_options.colors.is_none() { - return Some(&ColorsArg::Force); + if matches!(self, Self::Ci { .. }) && cli_options.colors.is_none() { + return Some(ColorsArg::Force); } // Normal behaviors - cli_options.colors.as_ref() + cli_options.colors.clone() } None => None, } }Nit: comment typo “give e better UX experience” → “give a better UX experience”.
🧹 Nitpick comments (1)
crates/biome_cli/src/cli_options.rs (1)
121-128: Helpers read clean; consider makingColorsArgtrivially copyable.
is_enabled/is_disabledare fine. Returning/passing colours by value elsewhere will be simpler ifColorsArgisCopy.Outside this hunk, add
Copy:// at line 115 #[derive(Debug, Clone, Copy)] pub enum ColorsArg { Off, Force, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/shy-ants-buy.md(1 hunks)crates/biome_cli/src/cli_options.rs(1 hunks)crates/biome_cli/src/commands/mod.rs(1 hunks)crates/biome_cli/src/logging.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changeset files, only use #### or ##### headers; avoid other header levels
Changeset descriptions should use past tense for what you did (e.g., "Added...")
Describe current Biome behavior in present tense within changesets (e.g., "Biome now supports...")
For bug fixes in changesets, start with a link to the issue (e.g., "Fixed #1234: ...")
When referencing rules or assists in changesets, include links to their documentation pages
Include a minimal code block in the changeset when applicable to demonstrate the change
End every sentence in the changeset description with a period
Files:
.changeset/shy-ants-buy.md
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_cli/src/commands/mod.rscrates/biome_cli/src/logging.rscrates/biome_cli/src/cli_options.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_cli/src/commands/mod.rscrates/biome_cli/src/logging.rscrates/biome_cli/src/cli_options.rs
🧠 Learnings (1)
📚 Learning: 2025-09-07T17:35:00.490Z
Learnt from: CR
PR: biomejs/biome#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-09-07T17:35:00.490Z
Learning: Applies to .changeset/*.md : Describe current Biome behavior in present tense within changesets (e.g., "Biome now supports...")
Applied to files:
.changeset/shy-ants-buy.md
🧬 Code graph analysis (1)
crates/biome_cli/src/cli_options.rs (1)
crates/biome_cli/src/logging.rs (1)
is_enabled(154-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed [#3824](https://github.com/biomejs/biome/issues/3824). Now the option CLI `--color` is correctly applied to logging too. |
There was a problem hiding this comment.
Correct the flag name and wording; match changeset style.
It’s --colors in the CLI, not --color. Also prefer the project voice and mention non‑TTY behaviour fixed by this PR.
-Fixed [#3824](https://github.com/biomejs/biome/issues/3824). Now the option CLI `--color` is correctly applied to logging too.
+Fixed #3824: Apply the CLI `--colors` option to logging. Biome now disables ANSI when output is not a TTY (for example when piped), unless `--colors=force` is set.
+
+```bash
+# colours disabled when piped
+biome check | cat
+# force colours
+biome check --colors=force
+```🤖 Prompt for AI Agents
.changeset/shy-ants-buy.md around line 5: update the changelog sentence to use
the correct CLI flag name `--colors` (not `--color`), rewrite in the project
voice and style, and mention the non‑TTY behaviour fix with the provided
examples; specifically, change the line to say that the `--colors` option is now
correctly applied to logging (including when output is piped), and add the two
example shells showing colours disabled when piped (`biome check | cat`) and
forcing colours (`biome check --colors=force`) formatted consistently with
existing changeset code blocks.
| cli_options.colors.as_ref(), | ||
| ); |
There was a problem hiding this comment.
Colour preference not applied: get_color() is ignored.
You compute CI/GitHub overrides in get_color() but pass cli_options.colors.as_ref() here, so the overrides never take effect.
Align with the updated logging signature:
- setup_cli_subscriber(
- cli_options.log_file.as_deref(),
- cli_options.log_level,
- cli_options.log_kind,
- cli_options.colors.as_ref(),
- );
+ let colors = self.get_color();
+ setup_cli_subscriber(
+ cli_options.log_file.as_deref(),
+ cli_options.log_level,
+ cli_options.log_kind,
+ colors,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cli_options.colors.as_ref(), | |
| ); | |
| let colors = self.get_color(); | |
| setup_cli_subscriber( | |
| cli_options.log_file.as_deref(), | |
| cli_options.log_level, | |
| cli_options.log_kind, | |
| colors, | |
| ); |
🤖 Prompt for AI Agents
In crates/biome_cli/src/commands/mod.rs around lines 853-854, the call is
passing cli_options.colors.as_ref() which ignores CI/GitHub overrides computed
by get_color(); update the call to pass the result of get_color() (e.g.,
get_color(&cli_options)) or otherwise call the new logging signature that
accepts the computed Color preference so the CI/GitHub override is applied
instead of the raw CLI option.
| pub fn setup_cli_subscriber( | ||
| file: Option<&str>, | ||
| level: LoggingLevel, | ||
| kind: LoggingKind, | ||
| colors: Option<&ColorsArg>, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accept colour preference by value to avoid lifetime foot‑guns.
Change setup_cli_subscriber to take Option<ColorsArg> (by value). This plays nicely with a value‑returning get_color() and avoids dangling refs.
-pub fn setup_cli_subscriber(
+pub fn setup_cli_subscriber(
file: Option<&str>,
level: LoggingLevel,
kind: LoggingKind,
- colors: Option<&ColorsArg>,
+ colors: Option<ColorsArg>,
) {🤖 Prompt for AI Agents
In crates/biome_cli/src/logging.rs around lines 14 to 19, the function signature
currently takes colors: Option<&ColorsArg>, which can lead to lifetime issues;
change the signature to accept colors: Option<ColorsArg> (by value). Inside the
function, update any use of colors to consume or clone the owned value (e.g.,
call get_color() that returns a ColorsArg) rather than borrowing, adjust call
sites to pass an owned ColorsArg or None, and remove any unnecessary lifetime
annotations related to colors; ensure all places that previously passed a
reference now pass the value (or call .cloned()/.to_owned() where appropriate).
Co-authored-by: siketyan <[email protected]> Co-authored-by: mdevils <[email protected]>
Summary
Closes #3824
Test Plan
Manually tested with and without
--colorsoption. IT works as expectedDocs