-
Notifications
You must be signed in to change notification settings - Fork 92
Apply colors only when they are supported #238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Apply colors only when they are supported #238
Conversation
pksunkara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we can just call the style_if_possible function as style. From a developer's perspective, they don't care whether that function is styling or not, depending on the env vars.
|
Pinging @epage so he can get more feedback on how |
99576ec to
a42ea90
Compare
pksunkara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please do the following changes:
- Replace all
owo_colors::style()usage withowo_colors::Style::new()(needed for 2nd change) - Rename all
style_if_possibletostyle.
|
You could pull out the choice logic, calling Or eyre could do something like clap and be neutral to this and assume the caller is using |
pksunkara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I will merge once you can get CI passing.
|
@synalice Can you please fix CI? |
|
Sure. I kind of forgot about that PR, sowwy. |
5584e56 to
60cdc40
Compare
|
Any updates on this ? |
|
I think I fixed all CI issue the last time I touched this MR. Not sure why it says "Some checks haven't completed yet" right now. |
|
Weird, can you rebase and push once? If it doesn't work, I will manually merge. |
6a09c09 to
a562f91
Compare
|
@pksunkara Any chance this can be merged? |
|
The CI is still failing with multiple different errors. |
a562f91 to
502c19b
Compare
|
CI still failing. @synalice I will be free this week to review and merge this if you are able to get the CI passing. |
502c19b to
642ebee
Compare
|
Still failing. Can you please try all the commands from CI locally first? Using github actions takes a lot of time since a maintainer needs to approve every time. |
Closes #236.
Closes #237.
This PR wraps all calls to
owo_colors::OwoColorize::stylewith function that callsanstream::AutoStream::choiceand checks whenever colors are available for the current stderr.Conserns
The stderr here is hardcoded it that somewhat bothers me. It is possible for that user of eyre to configure printing all of the error messages into the stderr? Because if eyre ever prints color to the stdout and then the user redirects stdout to the file, this check would detect that stderr is still a terminal and allow ANSI symbols to be printed to the file.
I am also wondering if duplicating
style_if_possibleincolor-spantraceandcolor-eyreis okay. Should I move it somewhere else?Tests
I've done some quick testing by running examples and this seems to work well. I don't see any colors in the terminal and there are no special symbols from eyre when redirecting to the file.
Here is a script to run all color-eyre examples and print the output to the file. Mind that
RUSTFLAGS=-Awarningswill force everything to rebuild. But that saves you from scrolling past all the deprecation warnings (fixed here btw — #235).Might be nice to write some unit tests, but I'd prefer if someone else did it.
About tracing_subscriber
The only ANSI symbols I see are the ones coming from tracing_subscriber (color-eyre/examples/theme_test_helper.rs:35). I did a little digging and found that removing this layer color-eyre/examples/theme_test_helper.rs:49 from registry fixes the issue.
The colors are still there, in the terminal, but not in the file. The only thing that changed visually is that the text is no longer cursive. Although I don't quite understand what this layer does and why is it needed. The documentation is not clear to me.
I think we should just remove this layer from every example to avoid teaching people this flawed behaviour. Let's leave it up to them to enable this layer. This way it won't be eyre's problem, but tracing_subscriber's one.
Rant about owo_colors
To be fair, this all feels like a huge blunder from owo_colors. I feel like it is it who should be responsible for detecting colors. They already have a
supports-colorfeature. But enabling it only gives you a method that you should apply everywhere yourself. I can understand why this feature isn't turned on by default, sure. But once I enable it I kind of expect everything to work out of the box. What if you have thousands of lines of code where colors are added? Do you wrap everything withif_supports_color??? This is nonsense.