fix: Match rustc's colors #73
Conversation
src/renderer/mod.rs
Outdated
| const BRIGHT_BLUE: Style = if cfg!(windows) { | ||
| AnsiColor::BrightCyan.on_default().effects(Effects::BOLD) | ||
| } else { | ||
| AnsiColor::BrightBlue.on_default().effects(Effects::BOLD) | ||
| }; |
There was a problem hiding this comment.
nit: as this isn't just bright blue, lets move the .effects call down below
src/renderer/mod.rs
Outdated
| } else { | ||
| AnsiColor::Yellow.on_default().effects(Effects::BOLD) | ||
| }, | ||
| info: BRIGHT_BLUE, |
There was a problem hiding this comment.
What does info map to in rustc? No level uses BRIGHT_BLUE. Is it a Secondary?
There was a problem hiding this comment.
It should map to UnderlineSecondary or LabelSecondary, but annotate-snippets doesn't have the concept of primary/secondary, so we print info: before it currently.
src/renderer/mod.rs
Outdated
| warning: if cfg!(windows) { | ||
| AnsiColor::BrightYellow.on_default().effects(Effects::BOLD) | ||
| } else { | ||
| AnsiColor::Yellow.on_default().effects(Effects::BOLD) |
There was a problem hiding this comment.
nit: should we have .effects be outside of the if expression?
|
My slight preference would be to externalize platform-specific settings to some options bag parameter like "use_cyan" and set it in constructor to true for windows in the binary. This makes it easier for a customer to replicate the same color scheme as windows has in other environments without having to emulate windows cfg. |
| line_no: AnsiColor::BrightBlue.on_default().effects(Effects::BOLD), | ||
| emphasis: Style::new().effects(Effects::BOLD), | ||
| line_no: BRIGHT_BLUE, | ||
| emphasis: if cfg!(windows) { |
There was a problem hiding this comment.
If I'm reading this correctly, this maps to MainHeaderMsg? What about Highlight?
There was a problem hiding this comment.
Correct, emphasis maps to MainHeadingMsg. As for Highlight annotate-snippets doesn't currently have anything for that concept (that I can tell).
src/renderer/mod.rs
Outdated
| AnsiColor::BrightWhite.on_default().effects(Effects::BOLD) | ||
| } else { | ||
| Style::new().effects(Effects::BOLD) | ||
| }, | ||
| none: Style::new(), |
There was a problem hiding this comment.
nit: should .effects be on the outside of the if?
This is all themable so they can do what they wish. For myself, my preference is that the agreed-to cross-Rust-project behavior be the default here so we don't have to worry about what feature flags and constructor parameters we need to keep in sync |
|
The goal for this change is to be as close to |
This PR makes it so that we match
rustc's output regarding colors. See here and here forrustc's current color implementation.Note: Where
rustcusesset_intense(true), that is the same asBright<color>inanstyle.Note: Most things are
BOLDasrustceventually sets most things bol (see the first link).