Owo color #2914
Owo color #2914
Conversation
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This ended up being the relatively nice workaround for jam1garner/owo-colors#123
| self.result = if field.name() == "message" { | ||
| self.record_debug_impl(field, &format_args!("{}", self.style.paint(value))) | ||
| } else { | ||
| self.record_debug(field, &value) | ||
| self.record_debug_impl(field, &self.style.paint(value)) |
There was a problem hiding this comment.
Is there a difference between the two calls? I wasn't sure, so I was afraid of touching this code.
| @@ -39,7 +39,7 @@ bytes = "1.2.0" | |||
| argh = "0.1.8" | |||
|
|
|||
| # sloggish example | |||
| nu-ansi-term = "0.46.0" | |||
| owo-colors = "4.0" | |||
There was a problem hiding this comment.
Has a very low MSRV (1.56), so it's a fine choice https://github.com/jam1garner/owo-colors/blob/master/Cargo.toml
There was a problem hiding this comment.
I don't know the library, but is it possible to somehow emulate the prefix/suffix pattern?
What I think will happen with this implementation is it will print for example the equivalent of <b>name</b><b>:</b> to print the colon in the same style as name.
That said it's a few more bytes for less allocations so still a net gain, just wondering.
| @@ -1688,7 +1719,7 @@ pub(super) mod test { | |||
| #[cfg(feature = "ansi")] | |||
| #[test] | |||
| fn with_ansi_true() { | |||
| let expected = "\u{1b}[2mfake time\u{1b}[0m \u{1b}[32m INFO\u{1b}[0m \u{1b}[2mtracing_subscriber::fmt::format::test\u{1b}[0m\u{1b}[2m:\u{1b}[0m hello\n"; | |||
| let expected = "\u{1b}[2mfake time\u{1b}[0m \u{1b}[32m INFO\u{1b}[39m \u{1b}[2mtracing_subscriber::fmt::format::test\u{1b}[0m\u{1b}[2m:\u{1b}[0m hello\n"; | |||
There was a problem hiding this comment.
From what I understand 39 sets the foreground colour to default while 0 resets everything to default. So I guess it is fine here if we don't have other modifiers, but 0 is safer if possible.
| @@ -403,24 +394,62 @@ impl<'a> PrettyVisitor<'a> { | |||
| } | |||
|
|
|||
| pub(crate) fn with_style(self, style: Style) -> Self { | |||
| Self { style, ..self } | |||
| if self.writer.has_ansi_escapes() { | |||
There was a problem hiding this comment.
I'm not sure this is correct. Cannot this be changed later by some calls like with_writer or set_ansi?
I am honestly unsure if these can be called after this point.
There was a problem hiding this comment.
PrettyVisitor takes ownership of the writer, and doesn't expose the writer. So I suppose it cannot be cannot be changed.
However, I agree that this isn't ideal code. If the writer were to ever get exposed, it'd be an potential bug.
There was a problem hiding this comment.
I changed it, let me know what you think.
|
@mladedav Thank you for the excellent review! To answer your question, yes, it is possible to get the prefix-suffix pattern to work. I had that, and removed it in this commit There's also an alternative approach, along the lines of struct FormatPair<'a, 'b, A, B>(&'a A, &'b B, Style);
impl fmt::Display for FormatPair {
// print the style prefix
// print a
// print b
// print the style suffix
}I could absolutely add one or more of those approaches, if the extra complexity is worth it. |
|
In case a reviewer is curious, I also tried out an alternative design, where a |
Motivation
Fix #2759
Also update docs to fix #2268
Solution
Replaced nu-ansi-term with owo-colors.
I then ran all the unit tests
cargo test, and tested thecargo run --example sloggishandcargo run --example fmt-prettyexamples. I did my best to verify that the output hasn't changed.Questions
During development, rust-analyzer always shows me bogus error messages and I'm not entirely sure what I'm doing wrong. I frequently ended up having to work with
cargo checkand a bit of guesswork.Old fmt-pretty output
New, visually unchanged fmt-pretty output