Skip to content
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

Better diff coloring #169

Merged
merged 3 commits into from Aug 25, 2012
Merged

Conversation

alexcoplan
Copy link
Contributor

Change configuration in RSpec::Matchers to use the same color config method as RSpec::Core (if core is available). Also made the diff coloring code more readable and succinct.

@travisbot
Copy link

This pull request passes (merged 248004e into d427bac).

}.each do |color_name, color_code|
define_method color_name do |text|
color(text, color_code)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I find this to be less readable and succinct than what you had before. I don't have a preference for color(text, red) over red(text) as an API (both seem equally fine to me), but using a hash and define_method adds extra indirection over what you had before. If you want the separate methods for each color, I'd prefer:

def red
  color(text, 31)
end

def green
  color(text, 32)
end

def blue
  color(text, 34)
end

I know on 1.8 that methods defined with define_method are slower when called than methods defined with def. Not sure about 1.9 (I haven't benchmarked it), but I expect it's still slower. So that's another reason to prefer def over define_method for this case.

@travisbot
Copy link

This pull request passes (merged 7cdd88b into d427bac).

@alexcoplan
Copy link
Contributor Author

@myronmarston OK, have changed it to multiple method defs.

myronmarston added a commit that referenced this pull request Aug 25, 2012
@myronmarston myronmarston merged commit f4d4cd0 into rspec:master Aug 25, 2012
@myronmarston
Copy link
Member

Thanks, I just merged it.

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.

None yet

3 participants