feat: modernize color composition and crate internals#5
Conversation
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR splits functionality into modules: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client/App
participant Config as ColorizeConfig
participant Env as NO_COLOR
participant TTY as TerminalDetector
participant Renderer as StyledText/Renderer
App->>Config: set_color_mode(mode)
App->>Renderer: create StyledText (text + styles/colours)
Renderer->>Config: should_colorize()?
Config->>Env: check NO_COLOR
alt NO_COLOR set
Env-->>Config: present
Config-->>Renderer: return false
else NO_COLOR not set
Config->>TTY: stdout.is_terminal()?
TTY-->>Config: true/false
Config-->>Renderer: return true/false (based on mode & TTY)
end
Renderer-->>App: render/Display (ANSI sequences or plain text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/color.rs (1)
31-42: Consider supporting 3-character shorthand hex codes.The implementation correctly parses 6-character hex codes, but does not support the common 3-character shorthand (e.g.,
#f80expanding to#ff8800). This is a minor usability gap that could be addressed in a follow-up.💡 Optional enhancement to support 3-char hex
pub(crate) fn hex_to_rgb(hex: &str) -> Option<(u8, u8, u8)> { let hex = hex.trim_start_matches('#'); - if hex.len() != 6 { - return None; - } + let hex = match hex.len() { + 3 => { + let chars: Vec<char> = hex.chars().collect(); + format!("{0}{0}{1}{1}{2}{2}", chars[0], chars[1], chars[2]) + } + 6 => hex.to_string(), + _ => return None, + }; - let r = u8::from_str_radix(&hex[0..2], 16).ok()?; - let g = u8::from_str_radix(&hex[2..4], 16).ok()?; - let b = u8::from_str_radix(&hex[4..6], 16).ok()?; + let r = u8::from_str_radix(&hex[0..2], 16).ok()?; + let g = u8::from_str_radix(&hex[2..4], 16).ok()?; + let b = u8::from_str_radix(&hex[4..6], 16).ok()?; Some((r, g, b)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/color.rs` around lines 31 - 42, The hex_to_rgb function currently only handles 6-character hex strings; update it to also accept 3-character shorthand by detecting when hex.len() == 3 and expanding it to 6 characters (duplicate each nibble, e.g., "f80" -> "ff8800") before calling u8::from_str_radix on the r/g/b slices; keep the existing trim_start_matches('#') behavior and reuse the same parsing logic (r/g/b via from_str_radix) so both 3- and 6-char inputs are supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 26-30: The TERMI NAL_OVERRIDE thread-local uses `const {
RefCell::new(None) }` which requires Rust 1.79+; update to be
backwards-compatible by replacing the const initializer with a non-const
initializer (i.e., use `RefCell::new(None)`) or alternatively declare the MSRV
by adding `rust-version = "1.79"` to Cargo.toml; edit the thread_local block
where `TERMINAL_OVERRIDE: RefCell<Option<bool>>` is defined (near `CONFIG` and
`ColorizeConfig::default`) to apply one of these two fixes so compilation works
on older toolchains.
In `@src/tests.rs`:
- Around line 40-56: The constructor currently hardcodes
previous_terminal_override = None instead of snapshotting the current
TERMINAL_OVERRIDE; add a test-only getter in src/config.rs (e.g.,
get_terminal_override_for_tests) that returns TERMINAL_OVERRIDE.with(|v|
*v.borrow()), then change the initialization in src/tests.rs to call that getter
so previous_terminal_override captures the real current value (mirror how
previous_mode and previous_no_color are captured) and keep
set_terminal_override_for_tests(terminal_override) unchanged.
---
Nitpick comments:
In `@src/color.rs`:
- Around line 31-42: The hex_to_rgb function currently only handles 6-character
hex strings; update it to also accept 3-character shorthand by detecting when
hex.len() == 3 and expanding it to 6 characters (duplicate each nibble, e.g.,
"f80" -> "ff8800") before calling u8::from_str_radix on the r/g/b slices; keep
the existing trim_start_matches('#') behavior and reuse the same parsing logic
(r/g/b via from_str_radix) so both 3- and 6-char inputs are supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ee8bfff-43f2-4dfb-b16e-0f15b744b071
📒 Files selected for processing (8)
README.mdTODO.mdexamples/basic.rssrc/color.rssrc/config.rssrc/lib.rssrc/style.rssrc/tests.rs
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tests.rs (1)
73-97: Comprehensive parametrised tests for colour and style rendering.Good use of
rstestfor data-driven tests covering all named colours, bright variants, and background colours. The pattern of matching on string case names works but could be simplified.♻️ Optional: Simplify with macro-generated tests
If the match arms become unwieldy, consider using a macro or mapping the colour name string directly to a method via a helper, though the current approach is perfectly readable.
Also applies to: 99-121, 123-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests.rs` around lines 73 - 97, The test_basic_colors function currently matches on string names to call methods like text.red(), text.green(), etc.; simplify and reduce repetition by creating a helper that maps the color name to the corresponding coloring function (or use a macro) and call it in the test, e.g., add a helper fn apply_color(s: &str, text: &str) -> String used by test_basic_colors instead of the long match; keep the TestStateGuard::colors_enabled(ColorMode::Always) setup and the existing rstest cases and assert_eq!(actual, expected) unchanged, only replace the match block with a single call to the helper/macro that selects and applies the correct method (red/green/yellow/blue/magenta/cyan/white/black).src/config.rs (1)
70-76:NO_COLORhandling is correct but consider documenting the precedence.The implementation correctly respects the
NO_COLORenvironment variable per the no-color.org specification. However, it's worth noting thatNO_COLORtakes precedence even whenColorMode::Alwaysis set—this might surprise users who expect "Always" to mean unconditional output.Consider adding a brief doc comment to
should_colorize()orColorMode::Alwaysclarifying thatNO_COLORstill disables output in that mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 70 - 76, Add a brief doc comment clarifying precedence of NO_COLOR: update the documentation on should_colorize() (or alternatively on the ColorMode::Always variant) to state that the NO_COLOR environment variable is respected and will disable color output even when ColorMode::Always is selected; reference ColorizeConfig::color_mode() and the should_colorize() behavior in the comment so readers know the implementation intentionally checks std::env::var_os("NO_COLOR") first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tests.rs`:
- Around line 46-49: The calls to env::set_var and env::remove_var in the match
on no_color must be wrapped in unsafe blocks to satisfy Rust 2024 rules; update
the match in tests.rs (where no_color is handled) so that both branches call
env::set_var(...) and env::remove_var(...) inside unsafe { ... }, keeping the
same values and behavior and ensuring any surrounding mutex/serialisation
remains intact.
---
Nitpick comments:
In `@src/config.rs`:
- Around line 70-76: Add a brief doc comment clarifying precedence of NO_COLOR:
update the documentation on should_colorize() (or alternatively on the
ColorMode::Always variant) to state that the NO_COLOR environment variable is
respected and will disable color output even when ColorMode::Always is selected;
reference ColorizeConfig::color_mode() and the should_colorize() behavior in the
comment so readers know the implementation intentionally checks
std::env::var_os("NO_COLOR") first.
In `@src/tests.rs`:
- Around line 73-97: The test_basic_colors function currently matches on string
names to call methods like text.red(), text.green(), etc.; simplify and reduce
repetition by creating a helper that maps the color name to the corresponding
coloring function (or use a macro) and call it in the test, e.g., add a helper
fn apply_color(s: &str, text: &str) -> String used by test_basic_colors instead
of the long match; keep the TestStateGuard::colors_enabled(ColorMode::Always)
setup and the existing rstest cases and assert_eq!(actual, expected) unchanged,
only replace the match block with a single call to the helper/macro that selects
and applies the correct method (red/green/yellow/blue/magenta/cyan/white/black).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bce24af-c6a6-4ea0-986b-53c7fde58367
📒 Files selected for processing (4)
README.mdTODO.mdsrc/config.rssrc/tests.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- TODO.md
| match no_color { | ||
| Some(value) => env::set_var("NO_COLOR", value), | ||
| None => env::remove_var("NO_COLOR"), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When did Rust mark env::set_var as unsafe and which edition requires unsafe blocks?
💡 Result:
Rust marked std::env::set_var as unsafe starting with the 2024 edition. In the 2024 edition, calls to set_var require an unsafe block, while previous editions (up to 2021) allow it without unsafe. This change was implemented via pull request #124636, merged around May 2024, to address soundness issues in multi-threaded programs on Unix-like systems due to POSIX setenv being MT-unsafe.
Citations:
- 1: https://doc.rust-lang.org/edition-guide/rust-2024/newly-unsafe-functions.html
- 2: https://doc.rust-lang.org/std/env/fn.set_var.html
- 3: Consider deprecating and/or modifying behavior of std::env::set_var rust-lang/rust#90308
- 4: Make
std::env::{set_var, remove_var}unsafe in edition 2024 rust-lang/rust#124636
Prepare for Rust 2024 edition: unsafe blocks required for env::set_var and env::remove_var
These functions require unsafe blocks in the 2024 edition due to soundness issues in multi-threaded contexts. Although the mutex serialisation in this code makes the operations safe in practice, you'll need to wrap these calls in unsafe blocks when adopting the 2024 edition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests.rs` around lines 46 - 49, The calls to env::set_var and
env::remove_var in the match on no_color must be wrapped in unsafe blocks to
satisfy Rust 2024 rules; update the match in tests.rs (where no_color is
handled) so that both branches call env::set_var(...) and env::remove_var(...)
inside unsafe { ... }, keeping the same values and behavior and ensuring any
surrounding mutex/serialisation remains intact.
There was a problem hiding this comment.
dont plan to adopt 2024 yet. I have already looked at the upgrade requirements and pros/cons - for now, it does not hurt this small library to stay on 2021
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
1 participant
Summary
This PR modernizes
colored_textand fixes a few bugsIt replaces nested ANSI string wrapping with a composed
StyledTextmodel, adds explicit runtime color modes with compatibility for the previous terminal-check API, and fixes the behavior ofclear()and invalid hex fallbacks so they return plain text.It also splits the crate into smaller modules, improves public rustdoc, tightens API ergonomics with
#[must_use]onStyledText, and adds targeted regression and coverage tests so the remaining uncovered paths are confined to test helpers rather than production behavior.Summary by CodeRabbit
New Features
Documentation
Tests
Chores