sharkdp
Enable custom colors with environment variables
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 https://github.com/sharkdp/hexyl/issues/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:
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.
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.
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):
BTW, I can pull https://github.com/sharkdp/hexyl/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!
Good call on the env var names. I've updated those all to use a HEXYL_COLOR_ prefix now.
