search

so-fancy / diff-so-fancy

Good-lookin' diffs. Actually… nah… The best-lookin' diffs. :tada:
MIT License
17.36k stars 335 forks source link

fix and maint: match most git supported color settings #453

Closed webstech closed 1 year ago

webstech commented 1 year ago

A patch series to incrementally expand support for git supported color settings.

  1. maint: set ansi colors in common routine Setting foreground and background colors is now using the same code.

  2. fix: ignore negative color attributes with tests The colors in the git config may have attributes and negative versions of them (no/no- prefix). The negative versions will no longer cause a false positive checking for the attribute. While not immediately obvious, upcoming support for default(ul) is also affected.

    A new test file is specifically for testing the settings of the ansi codes and the possible settings in the git config.

  3. fix: support git config colors normal and default Normal is a placeholder and will be ignored. Default will generate ansi codes for the default foreground or background color.

    Git validates the config so a bright prefix on these values would not be allowed. They are added to the hash to keep the code simple.

  4. fix: support #rgb colors in git config Git supports diff colors in #rrggbb notation. diff-so-fancy now supports them as well.

Some of the functions and constants in the head of test file git-config.bats can be moved to util.bash if that is preferred. They are not specific to that file. They are helpers to allow a more generic specification of the ansi escape codes without actually having to know the specific values.

webstech commented 1 year ago

This is a prelude to support for default and normal pseudo colors and rgb format colors. Decided to do a separate PR to allow any issues with the test file to be resolved first.

Edit: PR description and contents updated to include this support.

scottchiefbaker commented 1 year ago

I'll have to see how all the code pans out. I'm not gonna lie, this is a pretty low-demand feature request. If it's going to complicate the code and potentially hinder maintenance in the future I may pass on it.

My #1 goal with this project (and we're not there yet) is to make the code as readable as possible. The guts of d-s-f are pretty complex, and I'm hesitant to add MORE complexity for an issue (RGB colors) that only a handful of users have requested.

If we can keep the code clean and lower the maintenance cost I'm all for it.

webstech commented 1 year ago

I'll have to see how all the code pans out.

Updated the PR to go all the way with git config compatibility. RGB support was not a big change.

If we can keep the code clean and lower the maintenance cost I'm all for it.

The common routine for set_ansi_color removed duplicate code and made the last two commits simpler.

I know the new test script is a bit different with the tests but the intention is to make it easier to write tests without knowing the internal values for colors. QA test developers should not have to know all the internals.

scottchiefbaker commented 1 year ago

Circling back to this... this looks really good I like it and am gonna merge it.