fix: only emit ANSI color when stdout is a TTY - #551
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe package adds ChangesTerminal color support
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized CLI change gates ANSI color for non-TTY output, and no concrete user or production risk remains at the current head; it is merge-ready after normal checks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
==========================================
+ Coverage 98.13% 98.14% +0.01%
==========================================
Files 12 12
Lines 1337 1348 +11
==========================================
+ Hits 1312 1323 +11
Misses 25 25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Performance Changes
Comparing Footnotes
|
supports_color() from #551 answered FORCE_COLOR, the TTY and TERM, but not NO_COLOR — the variable users actually export globally to opt out of color (https://no-color.org). Any non-empty value now disables color, outranking detection and yielding only to an explicit FORCE_COLOR. Two gaps in the #551 tests are closed alongside. The reload-based tests recomputed the module constants under a patched environment and left the last reload's values in place for every test that ran afterwards; a fixture now re-derives them on teardown. And nothing exercised the copies commit_check.util binds at import — the ones the print functions actually read — so two subprocess tests now run the real import path end to end and assert on what gets printed.
supports_color() from #551 answered FORCE_COLOR, the TTY and TERM, but not NO_COLOR — the variable users actually export globally to opt out of color (https://no-color.org). Any non-empty value now disables color, outranking detection and yielding only to an explicit FORCE_COLOR. Two gaps in the #551 tests are closed alongside. The reload-based tests recomputed the module constants under a patched environment and left the last reload's values in place for every test that ran afterwards; a fixture now re-derives them on teardown. And nothing exercised the copies commit_check.util binds at import — the ones the print functions actually read — so two subprocess tests now run the real import path end to end and assert on what gets printed.



Problem
The CLI emitted raw ANSI color escapes unconditionally.
RED,GREEN,YELLOW, andRESET_COLORwere hardcoded escape sequences incommit_check/__init__.py, andprint_error_message/print_suggestionwrapped output in them without checking whether stdout is a terminal. Piped or
redirected output (CI logs, agent harnesses, files) therefore contained escape
garbage.
Fix
Add a
supports_color()probe that mirrors the existingsupports_hyperlinks()convention, and gate the constants themselves:FORCE_COLORoverrides in both directions (0off, anything else on),matching the
FORCE_HYPERLINKconvention already documented insupports_hyperlinks().stdoutdisables color.TERM=dumband an emptyTERM(TERM=) disable color — an empty valueis the same deliberate "no terminal type" signal as
dumb, while an unsetTERMstill allows color on a real terminal.Moving the decision to the constant definitions means
commit_check/util.pyis unchanged: the print functions keep interpolating the constants, which are
already empty when color is off.
Tests
supports_color()behavior acrossFORCE_COLOR, TTY, andTERMcases.Summary by CodeRabbit
New Features
Tests