Closed d4h0 closed 2 years ago
There isn't currently a method for this but I would be happy to merge support for customized color schemes and helpers to set useful defaults for dark and light backgrounds. This will probably involve a fair bit of work because color-eyre
has a panic and an eyre hook that would both need to leverage the color-scheme and this change would have to be propagated to color-spantrace
as well which is what currently colorizes the tracing::SpanTrace
output. I'm happy to mentor this if anyone wants to tackle adding the support.
Do you think this would be suitable for a beginner? I'm learning Rust for around 6 months now (I guess 3 or 4 full months of programming in Rust every day), but I still pretty much feel like a beginner... ;)
I also don't have any previous experience with system programming languages (so my code is most likely not the most performant code that could be created).
This will probably involve a fair bit of work
If the answer is "yes":
What do you think, how long this would take?
I'd guess a few hours, but I don't know the codebase, and what exactly needs to be changed.
Do you think this would be suitable for a beginner?
Yes, I can't be positive that something weird won't come up but I will help you work thru any issues and I don't forsee any.
What do you think, how long this would take?
A few hours sounds reasonable. Let me spec out the work that needs to be done and write up some detailed mentorship instructions, then we should have a good idea of what is needed.
Sounds good :)
Okay color-eyre
currently uses https://docs.rs/owo-colors/1.1.3/owo_colors/ to handle colorizing output it produces itself and it uses color-spantrace
for printing and colorizing tracing::SpanTrace
s which uses ansiterm
for colors I believe. To do this we're going to need to port color-spantrace
to use owo-colors
and switch to using dynamic colors for owo-colors
.
Steps:
color_eyre::config::ColorScheme
type to store the new color schemesHookBuilder::blank()
HookBuilder
for passing new ColorSchemes in to override the defaultcolor_scheme
member to the eyre and panic hooks created in HookBuilder::into_hooks
so they can use it when printing error reportsowo-colors
in color-eyre
to use the new ColorScheme
type to colorize output
color-backtrace
will be a useful reference for how this should work. https://github.com/athre0z/color-backtrace/blob/master/src/lib.rs#L389. The panic handler is actually forked from color-backtrace
and I ripped out the color scheme stuff because I didn't want to maintain it, oops :sweat_smile:.ColorScheme
type in the color-spantrace
crateansiterm
to use the new owo-colors
based ColorScheme
in color-spantrace
colorize
too color_spantrace::ColorScheme
for colorizing with custom color schemescolor_spantrace::colorize
From<color_eyre::config::ColorScheme> for color_spantrace::ColorScheme
to color-eyre
color_eyre::config::ColorScheme
into color-spantrace
when printing spantraces so the color style is consistent for the entire error report
And that should be everything. When you're working on this I recommend using a toml patch section which will let you make your local version of color-eyre
depend on your local version of color-spantrace
without having to make a new color-spantrace
release.
cc @jam1garner to verify that I didn't get any of the owo-colors
stuff wrong.
I think probably the best way to implement a ColorScheme
would be to have for it use OwoColorize::color
and OwoColorize::on_color
in place of whatever methods are currently being used in color-eyre (mostly likely the color-specific methods).
I would probably suggest waiting on jam1garner/owo-colors#3 getting resolved as I think that will make your life easier (and I am planning on handling today) as I would imagine you'd want to support Ansi, Xterm, and Rgb colors for the config?
I would imagine you'd want to support Ansi, Xterm, and Rgb colors for the config?
yea probably
@yaahc: Thanks for the detailed instructions! :)
Tomorrow I probably will be able to have a look. Depending on how much work this is, I most likely will stretch the work over several days.
@jam1garner: Thank you, the dynamic colors enum definitely will come in handy!
@d4h0 I have an initial version of the DynColors
enum up on the master branch of owo-colors in case you need it soon. I'd like a little bit more time to sit on the name and make sure it doesn't have any major issues before I push it to crates.io, but please feel free to comment in jam1garner/owo-colors#3 if you have naming suggestions or feel it otherwise needs work (as I don't particularly have a project at hand to test the design in a real situation).
feel free to comment in jam1garner/owo-colors#3 if you have naming suggestions or feel it otherwise needs work (as I don't particularly have a project at hand to test the design in a real situation).
@jam1garner: I will do that, thanks again.
@yaahc: Here is a short status update:
Today I mostly did the needed setup and made myself familiar with the code. My first problem was to find the places where owo_colors
is used. I ended up using ripgrep to find everything:
rg '\.(fg|bg|color|on_color|truecolor|on_truecolor|bold|dimmed|italic|underline|blink|blink_fast|reversed|hidden|strikethrough|((on_|bright_){0,1}(black|red|green|yellow|blue|magenta|purple|cyan|white)))'
(I'll paste the output in the next comment, in case you want to have a look).
Then I tried to find a way to get a string with color formatting that I can use for the test cases. So far I was not successful, but I should get there. If you know a simple way to get such a string, I'd be interested in this information, however.
The main reason why I send you this status report is that yesterday I have learned that I need to take care of a semi-emergency and that I, therefore, will not be able to do any programming for the next 7 days (I guess). After that, I will continue, however.
Here are the places where owo_colors
is used:
src/handler.rs
50: write!(indented(f).ind(n), "{}", error.bright_red())?;
src/section/help.rs
243: HelpInfo::Note(note) => write!(f, "{}: {}", "Note".bright_cyan(), note),
244: HelpInfo::Warning(warning) => write!(f, "{}: {}", "Warning".bright_yellow(), warning),
246: write!(f, "{}: {}", "Suggestion".bright_cyan(), suggestion)
259: write!(indented(f).ind(n), "{}", error.bright_red())?;
src/config.rs
57: write!(f, "{}", (&name[..name.len() - 19]).green())?;
59: write!(f, "{}", (&name[..name.len() - 19]).bright_red())?;
61: write!(f, "{}", (&name[name.len() - 19..]).bright_black())?;
77: filestr.purple(),
78: lineno.purple()
130: cur_line_no.white().bold(),
131: ">".white().bold(),
132: line.white().bold()
325: /// writeln!(f, "{}", "The application panicked (crashed).".red())?;
336: /// writeln!(f, "{}", payload.cyan())?;
341: /// write!(f, "{}", loc.file().purple())?;
343: /// write!(f, "{}", loc.line().purple())?;
504: writeln!(f, "{}", "The application panicked (crashed).".red())?;
515: writeln!(f, "{}", payload.cyan())?;
520: write!(f, "{}", loc.file().purple())?;
522: write!(f, "{}", loc.line().purple())?;
713: write!(&mut separated.ready(), "{:^80}", buf.bright_cyan())?;
If you know a simple way to get such a string, I'd be interested in this information, however.
Calling to_string()
on an eyre::Report
should give you the text of the report format including colors.
(I'll paste the output in the next comment, in case you want to have a look).
:+1:, alternatively you can look for files that include OwoColorize
because thats the extension trait that adds the various methods like purple()
to types that implement display, and if you comment out the use statement for OwoColorize
all of the usages of .cyan()
and such will become compiler errors.
The main reason why I send you this status report is that yesterday I have learned that I need to take care of a semi-emergency and that I, therefore, will not be able to do any programming for the next 7 days (I guess). After that, I will continue, however.
No worries, take all the time you need. Self care comes first!
I'm back! :)
Yesterday and today (so far) I've managed to create the test case (which I guess will be the most difficult part for me).
config::DefaultPanicMessage
can't be tested on stable, it seems (there is no way to create std::panic::PanicInfo
on stable).
Otherwise, my test covers all usages of owo_colors
in color_eyre
.
Is my assumption correct, that this test is only used to make sure my PR doesn't change the current default behavior and will be discarded before merging?
(If yes, you can skip to the next quote below)
Because my current approach is very fragile and can easily break if certain code of the standard library changes. To trigger is_dependency_code I have used an iterator while creating the error I will test against later. If that iterator code changes, the error I saved will not match future errors (because line numbers will be different).
But that isn't a problem if the compatibility test will be discarded anyway.
(I tried to create a dummy backtrace, but unsafe
seems to be needed, of which I know nothing. It also looks pretty difficult).
Calling to_string() on an eyre::Report should give you the text of the report format including colors.
to_string
seems to strip color information, but this led me to the solution (format!("{:?}", x)
works for some reason).
+1, alternatively you can look for files that include OwoColorize because thats the extension trait that adds the various methods like purple() to types that implement display, and if you comment out the use statement for OwoColorize all of the usages of .cyan() and such will become compiler errors.
Wow, I didn't think about that – thanks!
Otherwise:
As I wrote in the beginning, I guess creating the test was the most difficult part for me (it took me two days... x)). The rest looks pretty simple, but I guess this will still take more time for me than a few hours. I guess it will take 1-3 days.
In case you want to have a look, the test case is here. The code to generate/save errors is here (I used an external binary/example to make it easier to generate backtraces that stay compatible with previous ones).
I'm back! :)
Welcome back :D
to_string seems to strip color information, but this led me to the solution (format!("{:?}", x) works for some reason).
ooh yea I'd totally expect that in retrospect. to_string
calls Display
, but the colorized output from color-eyre
is produced by it's Debug
impl.
Is my assumption correct, that this test is only used to make sure my PR doesn't change the current default behavior and will be discarded before merging?
I'd love to get a nice test merged in, though like you've already noticed, these kinds of tests can be really fragile. We had one before that compared the exact output of the color codes much like how you're doing and I ended up removing it because it broke so often. If you wanted to try to get a version that is less fragile, possibly by just searching for the ordering of some ansi escape sequences, then I'd be super happy to merge that, but I definitely won't make this a requirement for merging. I understand it will probably be a lot of work so only do that if you want to.
(I tried to create a dummy backtrace, but unsafe seems to be needed, of which I know nothing. It also looks pretty difficult).
You might be able to get this to work by calling std::env::set_var("RUST_BACKTRACE", "1");
at the start of your test.
I'd love to get a nice test merged in
Okay, I will try a little longer to create a proper test case
If you wanted to try to get a version that is less fragile, possibly by just searching for the ordering of some ansi escape sequences
I think that's one thing I tried. I tried to remove all alphanum chars, but this also removed the ansi escape sequences. But it should be relatively easy to handle these ansi escape sequences somehow.
However, wouldn't that still break if there are changes to the external dependency (that triggers is_dependency_code?
We had one before that compared the exact output of the color codes much like how you're doing and I ended up removing it because it broke so often
Can you tell me about the ways it broke?
At the moment I only know about the above (if code that is in the backtrace changes (line number or the code itself), then the test breaks). I imagine that the test also could break if the hash at the end of the line changes (however, I know nothing about this hash, and it should be easy to filter this out).
Ideally, it would be possible to create a dummy Error
with a dummy Backtrace
– but it seems this is not possible because color_eyre
doesn't use Backtrace
s that are already attached to an Error
(see this and this – is this correct?).
If that all is true, then the only way to create a dependable test case (that I know about) would be to create a crate that contains a function that we can use to triggers is_dependency_code
.
Basically, a crate that contains:
// external_crate:
#[rustfmt::skip]
#[inline(never)]
pub fn apply<A, O>(f: fn(A) -> O, arg: A) -> O {
f(arg)
}
// Somewhere in the `color_eyre` test:
external_crate::apply(#[inline(never)] |msg| Report::msg(msg), "test"));
(Is there any other attribute that would be useful?)
Because external_crate
is only used in the test, it can just be a Git repo somewhere (right?). With external_crate
we then could guarantee that nothing ever will change.
However, this test case still will fail, if color_eyre
's formating changes.
To overcome this, I'd extract the ansi escape sequences (as you suggested). In combination with the "frozen" backtrace via the external crate, this should lead to a test case that doesn't break easily.
Do you see any way this test case could break, or do you have any suggestions?
If not, I'd go ahead and implement this before I continue.
(I tried to create a dummy backtrace, but unsafe seems to be needed, of which I know nothing. It also looks pretty difficult).
You might be able to get this to work by calling std::env::set_var("RUST_BACKTRACE", "1"); at the start of your test.
I think, you misunderstood me here. What I mean with "dummy traceback" is a traceback where I define the exact data it contains myself (fake data). This way it's possible to make 100% sure that the traceback is always the same.
Besides that:
What do you think about my proposal here?
With this it would be easy to let color_eyre
users define more than just colors.
To quote myself:
I believe this would give end-users great flexibility. For example, maybe there is a color_eyre user that would like to let the selected source line blink in bold red with a yellow background (😂), or use "hidden" on dependency code hashs.
However, wouldn't that still break if there are changes to the external dependency (that triggers is_dependency_code?
I think its okay if that breaks, if it becomes an issue we can just not test for that specific thing.
(see this and this – is this correct?).
Yea that's correct, for now color-eyre still uses backtrace-rs
rather than std::backtrace::Backtrace
so it can work on stable.
Can you tell me about the ways it broke?
The main issue was that the line numbers of the code in the tests kept changing, or whenever I tweaked the colors.
I think, you misunderstood me here. What I mean with "dummy traceback" is a traceback where I define the exact data it contains myself (fake data). This way it's possible to make 100% sure that the traceback is always the same.
Aaaah, makes sense. That sounds awesome.
What do you think about my proposal here?
:+1: https://github.com/jam1garner/owo-colors/issues/3#issuecomment-702272555
To overcome this, I'd extract the ansi escape sequences (as you suggested). In combination with the "frozen" backtrace via the external crate, this should lead to a test case that doesn't break easily.
Do you see any way this test case could break, or do you have any suggestions?
this sounds amazing. I can't think of any immediate issues with it.
I have implemented the new test case now (feel free to have a look, if you want).
My approach with the external crate didn't work (it seems external crates that are Git repos don't trigger is_dependency_code
), I went with Option::ok_or_else
instead. Because we now only check ANSI escape sequences, the test still should be pretty stable.
To extract the ANSI escape sequences I added a dev-dependency, I hope that is okay.
I had an idea about how to test config::DefaultPanicMessage
: We could create a small binary that panics and then execute it in a test with std::process::Command
and check stdout/stderr.
Would that be an acceptable approach?
I had an idea about how to test config::DefaultPanicMessage: We could create a small binary that panics and then execute it in a test with std::process::Command and check stdout/stderr.
sounds great! There might also be some shenanigans you can pull by wrapping the panic handler to redirect the output and catching it with catch_unwind
, but the sub binary sounds good too. Do whichever you feel would be easiest / best.
Edit: ooh it looks like your test already does catch_unwind, I'm confused how this doesn't already test the default panic message.
Btw, your test looks freaking amazing, great work
Edit: ooh it looks like your test already does catch_unwind, I'm confused how this doesn't already test the default panic message.
I'm assuming catch_unwind
runs before panic handlers (but I could be wrong). The information returned by catch_unwind
contains most of the error message that is printed by the panic handler. Just a few words are missing that are printed directly by DefaultPanicMessage
:
615: writeln!(f, "{}", "The application panicked (crashed).".red())?;
626: writeln!(f, "{}", payload.cyan())?;
631: write!(f, "{}", loc.file().purple())?;
633: write!(f, "{}", loc.line().purple())?;
But a test via a sub-binary will catch this.
Btw, your test looks freaking amazing, great work
Wow, thank you! 😄
At the moment I'm working on a PR for owo_colors
(I think I found a good solution now). From tomorrow or the day after, however, I need to do other stuff for a few days again. I will continue my work here after that.
I'm assuming
catch_unwind
runs before panic handlers (but I could be wrong). The information returned bycatch_unwind
contains most of the error message that is printed by the panic handler. Just a few words are missing that are printed directly byDefaultPanicMessage
:
ooooh, because you're converting it to a Report
and extracting that back out of the panic payload with downcast, that's why it's not showing the panic message, because you're only catching the value used to create the panic, but not the panic itself.
At the moment I'm working on a PR for owo_colors (I think I found a good solution now).
I just caught up on reading that issue, looks very promising.
From tomorrow or the day after, however, I need to do other stuff for a few days again. I will continue my work here after that.
Sounds good, take all the time you need, and thank you again for working on all of this.
ooooh, because you're converting it to a Report and extracting that back out of the panic payload with downcast, that's why it's not showing the panic message, because you're only catching the value used to create the panic, but not the panic itself.
Yes, exactly. As far as I can see, only what is passed to panic!
is available with catch_unwind
. Or do you mean there is a way to get the output from the panic handler? If I change the downcast to another type, unreachable!
is executed, and if I print out via debug
what catch_unwind
returns (without downcasting), only "Any" is printed.
Something I just realized:
In my implementation of Style
for owo_colors I don't preserve the order in which the user applies settings:
let x = Style::new().red().on_blue().apply_to("test");
let y = Style::new().on_blue().red().apply_to("test");
assert_eq!(x, y)
This means, most likely, that the current test case will fail.
Because I have ported the exact API of OwoColorize
to Style
, I think it's pretty unlikely that there will be any changes to the current default color scheme of color_eyre
.
I will just copy the code that currently invokes OwoColorize
methods to the new ColorScheme
type, and replace it with the identifier of the color scheme member.
Then, with the new color scheme functionality in place, I will generate new test data that can be used for future changes.
I currently assume the order of ANSI escape sequences is not important, so preserving the order would add unnecessary overhead to all users of owo_colors
.
Let me know if that is a problem.
Oops – false alarm!
The only relevant places are these:
cur_line_no.white().bold(),
">".white().bold(),
line.white().bold()
And they are in the right order for Style
... 😅
Yes, exactly. As far as I can see, only what is passed to
panic!
is available withcatch_unwind
. Or do you mean there is a way to get the output from the panic handler? If I change the downcast to another type,unreachable!
is executed, and if I print out viadebug
whatcatch_unwind
returns (without downcasting), only "Any" is printed.
The panic payload is set up differently depending on how panic!
is invoked. If you panic!
with a value directly that value will be the payload, if you panic with a message then the panic will be either a &str
or a String
I believe. Most panicking functions provided by std, such as unwrap
, do so with a message. So if you put a Report
in a Result::Err
and then unwrapped it and caught that message you'd get a string containing the message provided by unwrap
followed by the Debug repr of Report
which is just the report itself, so all the error messages and context that was captured. This however would still not be the output of the panic hook itself, so it doesn't help you test the panic message.
And here are some relevant resources if you're interested in learning how this works in more detail:
panic!
macro and converted into the type erased payloadThanks for the info, @yaahc! I have now implemented the panic test via an external process, and that works.
So if you put a Report in a Result::Err and then unwrapped it and caught that message you'd get a string containing the message provided by unwrap followed by the Debug repr of Report which is just the report itself
In the binary for the test, I pass Report
directly to panic, which results in a message of <non string panic payload>
, which is explained by your last message. However, that's probably better anyway, because Report
is already tested separately, and this way the panic message is small (i.e. easy to debug if something breaks).
Also: Finally, I managed to finished something that I can show you! :)
It turned out, the formats of the old color_eyre
and the new one are not compatible. Visually, everything looks the same, but the ANSI escape sequences are not identical:
ansi_parser
, the old version produces Escape(SetGraphicsMode([1]), Escape(SetGraphicsMode([37])
and my version produces Escape(SetGraphicsMode([37]), Escape(SetGraphicsMode([1])
. Somehow only "1" and "37" are switched. This doesn't seem to be a problem.)color_eyre
and color_spantrace
: For the active line color_eyre
uses .white().bold()
but color_spantrace
only uses .bold()
(which results in different output).I have compared the old and new output by eye and they are identical. I used diff tools to compare the ANSI escape sequences and found only the above differences (which seem to be negligible).
So what I propose is, to just discard the old test data, and use the new test data as control from now on.
At the moment I'm waiting for the review of my PR for owo_colors
, but it seems jam1garner is pretty busy at the moment.
Would you like to receive my PRs for color_eyre
and color_spantrace
now, or after my owo_colors
PR was accepted?
There were some problems with my initial PR, which I believe to have fixed. But I'm not sure if my code is good enough for owo_colors
(my code is working, but I have no clue if it's performant enough). So the worst case would be that my PR is rejected and a different API is chosen (by whoever implements the changes).
Personally, I don't care much if owo_colors
API changes, and I have to make a few changes, but maybe you think differently.
So what I propose is, to just discard the old test data, and use the new test data as control from now on.
Sounds good to me.
Would you like to receive my PRs for color_eyre and color_spantrace now, or after my owo_colors PR was accepted?
Now or whenever you're ready works for me, I don't mind reviewing the change in API later if that ends up being necessary, tho if it comes to that please make sure to bring in the new changes via a merge rather than a rebase so it doesn't erase the old history and prevent me from reviewing only the incoming changes.
Ok! Merged Style
into owo-colors. I haven't made a release (yet) as I'm still mulling over a solution to jam1garner/owo-colors#5 that isn't a breaking change... but if that ends up holding you up just @ me and I can make a new release whenever!
@jam1garner: Thanks again! :)
if that ends up holding you up just @ me and I can make a new release whenever!
For me personally, only the knowledge if the API is acceptable, was relevant.
@yaahc: Alright, I just opened my PRs :)
I believe this issue can be closed
Hi,
I'm using a light background in my terminal and some of the default colors are hard to read – is there a way to change the default color scheme?
BacktracePrinter has a color_scheme method, but I don't see a way to call this method or pass a custom
BacktracePrinter
to color-eyre.The Read Me says the following:
Would it be possible to either add a method to switch to a color scheme that's optimized for a light background, or add a method that lets the user do this?