Allow to define custom differ object in matchers #1096
Conversation
sponomarev
force-pushed
the
chore/custom-differs-from-matchers
branch
from
547fa8a
to
cc98519
Compare
There was a problem hiding this comment.
As this stands this changes a public API so cannot be merged at this point. I'm also not sure having a matcher level differ makes any sense, we're interested in building a new built in differ for RSpec 4, and I'm open to providing configurable differs globally, but per matcher has problems.
| # both present. | ||
| def fail_with(message, expected=nil, actual=nil) | ||
| def fail_with(message, matcher=nil) |
There was a problem hiding this comment.
I'm afraid you can't do this, it changes a public api.
| expected = "foo bar baz\n" | ||
| actual = "foo bang baz\n" | ||
| expected = "foo bang baz\n" | ||
| actual = "foo bar baz\n" |
There was a problem hiding this comment.
To prove your new implementation works, these should have been left alone.
| unless message | ||
| raise ArgumentError, "Failure message is nil. Does your matcher define the " \ | ||
| "appropriate failure_message[_when_negated] method to return a string?" | ||
| end | ||
|
|
||
| message = ::RSpec::Matchers::ExpectedsForMultipleDiffs.from(expected).message_with_diff(message, differ, actual) | ||
| message = if !matcher.nil? |
There was a problem hiding this comment.
As you can see above we already inject the differ, it just can't come from a param here, or at least, it'd have to be an extra param.
|
|
||
| ::RSpec::Matchers::ExpectedsForMultipleDiffs.from(matcher.expected).message_with_diff(message, differ_in_use, matcher.actual) | ||
| else | ||
| ::RSpec::Matchers::ExpectedsForMultipleDiffs.from(nil).message_with_diff(message, differ, nil) |
There was a problem hiding this comment.
I'm yeah nahing this, expected and actual need to be passed in, differ could be an optional extra, this would eliminate the need for this if.
| |-["poo", "car"] | ||
| |+[(a string matching /foo/), (a string matching /bar/)] | ||
| |-[(a string matching /foo/), (a string matching /bar/)] | ||
| |+["poo", "car"] |
There was a problem hiding this comment.
To be backwards compatible, existing specs shouldn't change...
|
Would you be interested in taking another approach to the problem @sponomarev ? |
|
I am interested to work on this if @sponomarev can not, after I finish the task on documentation and yard. rspec/rspec.github.io#131 |
|
@pirj Yeah, for sure. I'll take care of it this week. @benoittgt thanks for the offer, I think I'll deal with it on my own =) |
|
@sponomarev A friendly reminder |
|
@sponomarev Little bump on this one 😊 |
The questions about custom differ already appeared in the past in #627, rspec/rspec#76. I think it could be valuable to allow custom differ objects in matchers.
The proposed solution should be completely backward compatible. And, I hope, will provide a way to extend custom RSpec matchers libraries.