splitwise / super_diff

A more helpful way to view differences between complex data structures in RSpec.
https://splitwise.github.io/super_diff/
MIT License
992 stars 54 forks source link

Bright versions of colours output incorrectly #125

Open unikitty37 opened 3 years ago

unikitty37 commented 3 years ago

Setting any colour to its bright_ variant produces incorrect output in super_diff 0.6.1.

Running this test

RSpec.describe SuperDiff do
  it 'deliberately fails' do
    expected = { foo: 1, bar: 2, baz: 3 }
    actual = { bar: 2, baz: 2, quux: 4 }

    expect(actual).to eq(expected)
  end
end

with this config

SuperDiff.configure do |config|
  config.expected_color = :red
end

changes the expected colour to red as expected:

image

If the config is changed to use :bright_red instead of :red, the diff lines are coloured correctly, but the key and the Expected section instead output right_redm instead of changing the colour:

image

I've been able to repeat this with bright_ variations of other colours such as green, black, and blue β€” though I haven't tried yellow, cyan, white, and magenta, I suspect it will affect them too 😁

Output from `bundle list`

``` Gems included by the bundle: * actioncable (6.1.3) * actionmailbox (6.1.3) * actionmailer (6.1.3) * actionpack (6.1.3) * actiontext (6.1.3) * actionview (6.1.3) * activejob (6.1.3) * activemodel (6.1.3) * activerecord (6.1.3) * activestorage (6.1.3) * activesupport (6.1.3) * addressable (2.7.0) * amazing_print (1.2.1) * ast (2.4.2) * attr_extras (6.2.4) * bcrypt (3.1.16) * binding_of_caller (0.8.0) * brakeman (5.0.0) * builder (3.2.4) * bullet (6.1.3) * bundler (2.0.2) * bundler-audit (0.8.0.rc1) * bundler-leak (0.2.0) * byebug (11.1.3) * claide (1.0.3) * claide-plugins (0.9.2) * coderay (1.1.3) * colored2 (3.1.2) * concurrent-ruby (1.1.8) * cork (0.3.0) * crack (0.4.5) * crass (1.0.6) * danger (8.2.2) * danger-rubocop (0.9.3) * database_consistency (0.8.13) * debug_inspector (0.0.3) * devise (4.7.3) * devise-jwt (0.8.1) * diff-lcs (1.4.4) * docile (1.3.2) * dry-auto_inject (0.7.0) * dry-configurable (0.12.1) * dry-container (0.7.2) * dry-core (0.5.0) * erubi (1.10.0) * et-orbi (1.2.4) * factory_bot (6.1.0) * factory_bot_rails (6.1.0) * faraday (1.3.0) * faraday-http-cache (2.2.0) * faraday-net_http (1.0.1) * ffi (1.14.2) * friendly_id (5.4.2) * fugit (1.4.2) * fuubar (2.5.0) * geocoder (1.6.5) * git (1.8.1) * gitlab (4.17.0) * globalid (0.4.2) * hashdiff (1.0.1) * httparty (0.18.1) * i18n (1.8.9) * icalendar (2.7.0) * ice_cube (0.16.3) * image_processing (1.12.1) * jbuilder (2.11.2) * jwt (2.2.2) * kramdown (2.3.0) * kramdown-parser-gfm (1.1.0) * kwalify (0.7.2) * launchy (2.5.0) * listen (3.2.1) * logstop (0.2.6) * loofah (2.9.0) * mail (2.7.1) * marcel (0.3.3) * method_source (1.0.0) * mime-types (3.3.1) * mime-types-data (3.2021.0212) * mimemagic (0.3.5) * mini_magick (4.11.0) * mini_mime (1.0.2) * mini_portile2 (2.5.0) * minitest (5.14.4) * mono_logger (1.1.0) * multi_json (1.15.0) * multi_xml (0.6.0) * multipart-post (2.1.1) * mustermann (1.1.1) * nap (1.1.0) * nio4r (2.5.5) * no_proxy_fix (0.1.2) * nokogiri (1.11.1) * octokit (4.20.0) * open4 (1.3.4) * orm_adapter (0.5.0) * parallel (1.20.1) * parser (3.0.0.0) * patience_diff (1.1.0) * pg (1.2.3) * pry (0.13.1) * pry-byebug (3.9.0) * pry-doc (1.1.0) * pry-rails (0.3.9) * pry-theme (1.3.0) * psych (3.3.0) * public_suffix (4.0.6) * puma (4.3.5) * pundit (2.1.0) * pundit-matchers (1.6.0) * raabro (1.4.0) * racc (1.5.2) * rack (2.2.3) * rack-cors (1.1.1) * rack-protection (2.0.8.1) * rack-test (1.1.0) * rails (6.1.3) * rails-dom-testing (2.0.3) * rails-html-sanitizer (1.3.0) * railties (6.1.3) * rainbow (3.0.0) * rake (13.0.3) * rb-fsevent (0.10.4) * rb-inotify (0.10.1) * rchardet (1.8.0) * redis (4.2.5) * redis-namespace (1.8.1) * reek (6.0.3) * regexp_parser (2.1.1) * rein (5.1.0) * responders (3.0.1) * resque (2.0.0) * rexml (3.2.4) * rspec-collection_matchers (1.2.0) * rspec-core (3.10.1) * rspec-expectations (3.10.1) * rspec-mocks (3.10.2) * rspec-rails (4.0.2) * rspec-support (3.10.2) * rspec_junit_formatter (0.4.1) * rubocop (1.10.0) * rubocop-ast (1.4.1) * rubocop-performance (1.9.2) * rubocop-rails (2.9.1) * rubocop-rspec (2.2.0) * rubocop-thread_safety (0.4.2) * ruby-progressbar (1.11.0) * ruby-vips (2.0.17) * ruby2_keywords (0.0.2) * rufus-scheduler (3.7.0) * sawyer (0.8.2) * sentry-raven (3.1.1) * simplecov (0.18.5) * simplecov-html (0.12.2) * sinatra (2.0.8.1) * sitemap_generator (6.1.2) * sprockets (4.0.2) * sprockets-rails (3.2.2) * strong_migrations (0.7.6) * super_diff (0.6.1) * terminal-table (1.6.0) * thor (1.1.0) * tilt (2.0.10) * trollop (1.16.2) * tzinfo (2.0.4) * unicode-display_width (2.0.0) * uniform_notifier (1.13.2) * validate_url (1.0.13) * vegas (0.1.11) * warden (1.2.9) * warden-jwt_auth (0.5.0) * webmock (3.11.2) * websocket-driver (0.7.3) * websocket-extensions (0.1.5) * yard (0.9.25) * zeitwerk (2.4.2) ```

mcmire commented 3 years ago

Whoops! It's possible that I haven't completely tested this. I will look at this shortly!

mcmire commented 3 years ago

I looked into this briefly and there's two things going on here. First, super_diff uses RSpec's color set to colorize some pieces of the output and its own color set to colorize other pieces. Second, super_diff patches RSpec so that if it looks up a color in RSpec's color set and can't find anything, it just uses the input anyway. I can't remember why I did this exactly, but that's why you're getting garbage in the output.

So I think in order to fix this, that patch probably needs to be reverted, and there needs to be one way to color text and not two. At the moment I am leaning toward constraining super_diff's colorization capabilities to match those of RSpec's, to limit the amount of custom stuff super_diff does. That means that bright colors would not be supported in super_diff. That said, if you feel like bright colors is something you really want then I could be convinced otherwise!

unikitty37 commented 3 years ago

Thanks β€”Β the main problem is that only some of the dark colours are readable on a black background on my terminal.

I could redefine the colours in iTerm 2's config, but as I'm using its 256 colour support, I'm not sure if that even works. (Support for the colours of xterm-256 would be lovely, but probably a bit much to hope for :)

That said, the main colour I was trying to change was the border colour, so it's not essential in my case, as the actual diffs are readable.