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

Add support for choosing differs #97

Open
dchelimsky opened this issue Nov 24, 2011 · 7 comments
Open

Add support for choosing differs #97

dchelimsky opened this issue Nov 24, 2011 · 7 comments
Labels

Comments

@dchelimsky
Copy link
Contributor

We've had a few issues and pull requests dealing with diffing, but I think we need a more holistic solution. There are a growing number of diffing solutions out there now, each of which are going to appeal more to some people than others, so I think we should have a reasonable default behavior, but then expose an API for choosing your own differ. Something like:

RSpec.configure do |c|
  c.diff {|actual,expected| MyPreferredDefaultDiffer.diff(actual, expected)} # default
  c.diff(Hash)   {|expected,actual| MyPreferredHashDiffer.diff(expected, actual)}
  c.diff(Array)  {|expected,actual| MyPreferredArrayDiffer.diff(expected, actual)}
  c.diff(String) {|expected,actual| MyPreferredStringDiffer.diff(expected, actual)}
  # etc
end

The form with a single argument would invoke the supplied block if both expected and actual values match the argument, otherwise the default would be used.

@dchelimsky
Copy link
Contributor Author

See #79 for some background info on this.

@myronmarston
Copy link
Member

I like this API. For maximum flexibility, I think this should use the === operator to match the argument to expected and actual, rather than something like is_a?.

@dchelimsky
Copy link
Contributor Author

Agreed.

@dchelimsky
Copy link
Contributor Author

I'd like to have solid defaults as well. The question is whether to make them hard dependencies (as we do now with diff-lcs) or make it more like if defined?(SomeDiffer), then use it, otherwise ..... Thoughts on that?

@myronmarston
Copy link
Member

I think most rspec users (including myself) don't have strong opinions about what differ they want to use, but they do want a decent option that immediately works when they gem install rspec. I've also never heard complaints about the diff-lcs dependency.

Thus, I think it's good to keep diff-lcs (or something similar) as a hard dependency.

@playupchris
Copy link

As (another) rspec user I want something that will (predictably) work out of the box.

The issue I was trying to solve with http://github.com/playup/diff_matcher in the first place, was that rspec doesn't have a JSON matcher (or a decent hash matcher that can be used to to perform that match).

diff_matcher ended up (by accident) growing into more than just a hash matcher, but it isn't a replacement for diff-lcs.

And it actually behaves differently to the match_array built in to rspec.

[1,2,3].should   =~ [2,3,1]

(This would pass in match_array, but fail in diff_matcher; which IMHO is the correct behaviour, but to maintain backward compatibility would mean that diff_matcher can't be used as a replacement for match_array.)

We're happy with the diff_matcher gem at our company and use it in rspec with our own custom matcher to match JSON.

Using something like the rspec configuration API above could be a nice compromise, but since JSON is actually a String we'd still need our own custom matcher to match JSON anyway.

@dchelimsky
Copy link
Contributor Author

@playupchris:

  • we can still add be_json_matching (JSON.parse(actual).should eq(expected))
  • we can use diff_matcher for hashes and arrays, but still use diff-lcs for strings and other objects
  • [1,2,3].should =~ [2,3,1] would still work the way it does now (actual.sort.should eq(expected.sort))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants