Skip to content

Only disable color when NO_COLOR is non-empty - #1976

Open
waldekmastykarz wants to merge 1 commit into
dotnet:mainfrom
waldekmastykarz:fix-1967-no-color-empty
Open

waldekmastykarz wants to merge 1 commit into
dotnet:mainfrom
waldekmastykarz:fix-1967-no-color-empty

Conversation

@waldekmastykarz

Copy link
Copy Markdown
Collaborator

Closes #1967

Per no-color.org, NO_COLOR should disable color only when present and not empty. Previously any NO_COLOR value, including empty, disabled color.

  • Extracted the check into an internal ShouldDisableColor helper using string.IsNullOrEmpty.
  • Added unit tests covering NO_COLOR (unset/empty/non-empty), --no-color, and TERM=dumb.

Per https://no-color.org, NO_COLOR applies only when present and not empty.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 09b222bb-b264-4461-926d-32f935613554
@waldekmastykarz
waldekmastykarz requested a review from a team as a code owner October 21, 2026 13:54
Copilot AI balanced review requested due to automatic review settings October 21, 2026 13:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused fix meets the requirement, preserves existing overrides, and includes appropriate regression tests.

No open issues 🎉

What changed in this PR

Fixes #1967 by keeping color enabled when NO_COLOR is empty, matching the documented standard.

Changes:

  • Extracts color-disable logic into a testable helper.
  • Preserves --no-color and TERM=dumb behavior.
  • Adds regression tests for environment variables and the command-line override.
File Description
DevProxy/​Commands/​DevProxyCommand.cs Requires a non-empty NO_COLOR to disable color.
DevProxy.Tests/​DevProxyCommandNoColorTests.cs Tests environment values and existing overrides.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused fix meets the requirement, preserves existing overrides, and includes targeted regression tests.

No open issues 🎉

Review effort: Balanced

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.

NO_COLOR with an empty value disables color

2 participants