Skip to content

Enable custom colors with environment variables (PoC) #248

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dan-hipschman
Copy link

Hello!

I really like hexyl but the default colors of green and cyan look too similar on my terminal (first output in screenshot below). I was surprised they aren't already customizable (there's issue #220 for this), so I took a stab at enabling customization via env vars. It turned out to not be as straightforward as I'd hoped. hexyl uses the terminal codes directly, and the owo_colors API for dynamic colors expects you to use the fmt traits. In other words, DynColors doesn't provide direct access to the terminal codes like const colors::*::ANSI_FG does. So, in short, it required a slightly ugly hack to get working, without rewriting a lot of other code anyway.

For that reason I'm submitting this PR as a proof of concept. If you are OK with it then I can finish it up with documentation and tests, etc.

Screenshot:
Screenshot from 2025-05-12 14-14-00

@dan-hipschman dan-hipschman force-pushed the custom-colors-from-env branch from 65af6c9 to 6bfce85 Compare May 12, 2025 21:18
Copy link
Contributor

@sharifhsn sharifhsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great work, and as you've noted a popularly requested feature. In my opinion, this is high quality enough to be merged.

On your hack: since you are formatting at initialization time (good use of LazyLock!), this seems perfectly acceptable. owo-colors does expose a way to output raw ANSI codes through fmt_raw_ansi_fg, but it requires an instance of Formatter since it's meant to be used in a Display implementation context. Stable Rust does not currently provide any way to construct a Formatter (there is an unstable implementation). owo-colors also has a sealed trait that directly exposes the raw color codes as a constant. You can raise an issue with owo-colors to add this to their API surface.

@sharkdp There seems to be some issues with the CI in hexyl. The Ubuntu version for all of the Linux runners is deprecated, and the pc-windows-gnu seems to not be able to run at all.

@dan-hipschman
Copy link
Author

Thanks for the review @sharifhsn! Ok if this approach is fine then I can polish it up and include docs and tests. Just to add a little more about the Color trait in owo-colors: as you mentioned, it does have ANSI_FG, but DynColors doesn't implement this trait. And given ANSI_FG is a const, it really can't be sanely be implemented for dynamic colors. So, yea, they would need to add a new API for it. I can raise an issue with them.

Re CI: I noticed this too and started looking at it. The ubuntu jobs should be easy to fix since they're caused by Github deprecating an old runner image so just needs an update. I couldn't figure out why the windows build is failing. The MSVC one works, it's only the GNU one that fails. I can look into it a little more, but no promises I can figure it out since I'm not a Windows user.

@dan-hipschman
Copy link
Author

I made a separate PR to fix CI, although I couldn't figure out the Windows build issue so just removed that target from the tests. I added some notes in case anyone else wants to debug.

@dan-hipschman dan-hipschman force-pushed the custom-colors-from-env branch from 6bfce85 to 69b63a2 Compare May 15, 2025 02:26
@dan-hipschman
Copy link
Author

Ok, this should be ready for review!

I updated docs and added tests. I noticed there are currently no tests for color. I assume this might be because strings with color codes in them are near impossible to read, so tests with these expected outputs as strings are ugly at best, and finicky / error prone at worst (and maybe there's portability issues too?). Anyway, after a few iterations, I think I have a pretty neat solution which is described in the test. It keeps tests intuitive to read, and easy to get the control you need. It's admittedly a little fancy for a test framework (I subscribe to keeping test code as simple as possible), but I think it's worth it in this case. Let me know what your thoughts are.

Here's a sample of what the test would look like when it fails (changed the offset color from red to green):

Screenshot from 2025-05-14 19-39-44

BTW, I can pull #249 into this PR if you want to ensure tests pass in CI. And I can add more tests if you'd like. I added a few but since there were none to begin with I could definitely add more. I just didn't want to go crazy until hearing back. Looking forward to feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants