Colored Diffs #157

Merged
merged 5 commits into from Jul 18, 2012

Conversation

Projects
None yet
4 participants
Contributor

alexcoplan commented Jul 17, 2012

This adds support for diffs to be colored (like git with git config ui.color true). Respects the --color option in rspec config or passed at command-line.

  • Added support for colored diffs with helpers.
  • color option now configurable within RSpec::Matchers.configuration
  • Added contextual examples for --color enabled/disabled in the differ spec.

This pull request fails (merged 505db9f into 2c2fdb6).

Contributor

alexcoplan commented Jul 17, 2012

I'm guessing that it might be failing my new example because under certain environments, it won't allow color output. Interesting that 1.8.7, jruby and ree are the ones that are failing. I'm looking into it.

@myronmarston myronmarston and 1 other commented on an outdated diff Jul 17, 2012

lib/rspec/expectations/differ.rb
@@ -62,6 +63,27 @@ def context_lines
3
end
+ def color(text, code)
+ "#{code}#{text}\e[0m"
+ end
+
+ def color_diff(diff)
+ return diff unless RSpec::Matchers.configuration.color?
+ lines = diff.lines.map do |line|
+ case line[0]
+ when "+"
+ color(line, "\e[33m") # yellow
@myronmarston

myronmarston Jul 17, 2012

Owner

I'm curious why you chose yellow? Most diff tools use green.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

Most GUI diffs use green. For the CLI I chose yellow (like git), because lots of people use green as their default terminal color.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

Oh wait, git might actually be using a very bright yellowy green - let me know if you would prefer a different color code.

@myronmarston

myronmarston Jul 17, 2012

Owner

My git output uses green. I think green makes more sense.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

Done in latest commit.

@myronmarston myronmarston and 2 others commented on an outdated diff Jul 17, 2012

lib/rspec/expectations/differ.rb
@@ -62,6 +63,27 @@ def context_lines
3
end
+ def color(text, code)
+ "#{code}#{text}\e[0m"
+ end
+
+ def color_diff(diff)
+ return diff unless RSpec::Matchers.configuration.color?
+ lines = diff.lines.map do |line|
+ case line[0]
+ when "+"
+ color(line, "\e[33m") # yellow
+ when "-"
+ color(line, "\e[31m") # red
+ when "@"
+ line[1] == "@" ? color(line, "\e[34m") : line # blue
@myronmarston

myronmarston Jul 17, 2012

Owner

Rather than using the magic numbers and comments to indicate what the color is, I'd prefer to see constants (e.g. RED = 31).

Also, if the color method did something like "\e[#{code}m#{text}\e[0m" then your the code here that uses it could read like color(line, RED) which is more clear and readable, I think.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

Yep, will change this. I did take a look at how RSpec::Core does colors and it uses helpers for each color individually, which I thought may have been slight overkill for this.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

Fixed in latest commit.

@adamcooke

adamcooke Jul 18, 2012

The comment should probably read "probably yellow" seeing 33 is quite brown-ish on some terminals 😄

@alexcoplan

alexcoplan Jul 18, 2012

Contributor

@adamcooke indeed.. green looks surprisingly yellow on mine!

Owner

myronmarston commented Jul 17, 2012

I'm looking into it.

Great, thanks :).

This pull request fails (merged 477fac1 into 2c2fdb6).

@alexcoplan alexcoplan commented on the diff Jul 17, 2012

spec/rspec/expectations/differ_spec.rb
@@ -149,5 +155,23 @@ def inspect
end
end
end
+
+ context "with --color" do
+ before { RSpec::Matchers.configuration.stub(:color? => true) }
+
+ let(:differ) { RSpec::Expectations::Differ.new }
+
+ it "outputs colored diffs" do
+ expected = "foo bar baz"
+ actual = "foo bang baz"
+ expected_diff = "\n\e[34m@@ -1,2 +1,2 @@\n\e[0m\e[31m-foo bang baz\n\e[0m\e[32m+foo bar baz\n\e[0m"
+
+
+ diff = differ.diff_as_string(expected,actual)
+ diff.should == expected_diff
@alexcoplan

alexcoplan Jul 17, 2012

Contributor

This is the failing test on travis. It fails under 1.8.7, jruby and ree. For some reason, diffs are not even getting touched, even though I have directly stubbed RSpec::Matchers.configuration.color?. This is very odd, as you can see if you look in differ.rb at line 71, diffs are returned only if RSpec::Matchers.configuration.color? returns false.

I'm currently installing 1.8.7 to play around and try and work out why this is breaking.. if anyone has any bright ideas, let me know.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

OK, have replicated locally with 1.8.7. Currently investigating.

@alexcoplan

alexcoplan Jul 17, 2012

Contributor

Should now be fixed. The problem was that older Ruby implementations don't directly let you access string characters like str[0], you get the character's ascii ordinal instead, so in the switch statement none of my checks for @, + or - were matching. With str[0].chr, this should work across all supported implementations.

This pull request passes (merged 83cdba4 into 2c2fdb6).

Contributor

alexcoplan commented Jul 17, 2012

@myronmarston It's passing now :) I was incorrectly handling characters for older Ruby implementations.

myronmarston merged commit 3b4f60c into rspec:master Jul 18, 2012

Owner

myronmarston commented Jul 18, 2012

Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment