Extract differ to support #441
Conversation
|
Thanks for working on this, @phiggins. I've got a backlog of other PRs I'm reviewing and helping with so it might be a few days before I get to this. |
|
No rush @myronmarston, I just wanted to get this up before too much time had passed. |
|
I'd feel more comfortable with this if the differ was configurable, and set to the support differ by default, that way you could inject a double rather than stubbing the instantiation of the differ. |
| module RSpec | ||
| module Expectations | ||
| class << self | ||
| # @private | ||
| def differ | ||
| DiffPresenter.new | ||
| RSpec::Support::DiffPresenter.new( |
There was a problem hiding this comment.
I'd also rather this took definitive options rather than a hash...
There was a problem hiding this comment.
What do you mean by "definitive options"?
There was a problem hiding this comment.
Arguments I guess, see the rspec/rspec-support#36 for further discussion of this though
That is a great idea, will do. |
| module RSpec | ||
| module Expectations | ||
| class << self | ||
| # @private | ||
| def differ | ||
| DiffPresenter.new | ||
| RSpec::Support::DiffPresenter.new( | ||
| stringifier: lambda {|object| RSpec::Matchers::Composable.surface_descriptions_in(object) }, |
There was a problem hiding this comment.
I'm not sure that stringifier is the right name for this -- it suggests that the lambda returns a string, and this lambda doesn't: it returns the same kind of object (such as an array or hash) with any matchers appropriately replaced with objects whose inspect returns the description of the matcher.
Maybe object_preparer is a better name? It prepares objects for being diffed.
I prefer testing the actual integration. The external behavior for the user is the final error message that is produced. The options passed to achieve that are an implementation detail that we may refactor. |
|
There's still a bit of work to be done on both tickets so I might not get this done immediately. Thanks so much for the feedback guys! |
|
I think I have addressed all of the feedback given. What do you folks think? |
|
Needs rebasing for a start... ;) |
| @@ -1,9 +1,14 @@ | |||
| require 'rspec/support/differ' | |||
There was a problem hiding this comment.
This should leverage our new require stuff.
|
Looking good, needs rebasing, squashing, and a minor update. We also need the support side merged first. |
|
This has been rebased. |
|
Still needs squashing please :) |
|
I pushed some changes to rspec-support that should fix the build failures, but I can't as a non-repo owner force travis to rebuild. :/ |
|
Squashed this one as well. |
|
Travis says the build failed, but these all look fine to me... ? https://travis-ci.org/rspec/rspec-expectations/jobs/21604745 |
|
I've merged support so can you drop the references to your branch? |
| @@ -1,9 +1,14 @@ | |||
| RSpec::Support.require_rspec_support('differ') | |||
|
LGTM, as soon as you've restored the support dependency and it goes green I'll merge. |
|
Cool, it's in the middle of my work day right now so it'll be a few hours. |
|
No worries, it's almost the start of my work day ;) |
|
Just ping a comment when you've done it, as github doesn't notify us of commits |
|
Will do! |
|
The support dependency is back to normal and I removed the extra parenthesis you noted. Travis is running but it looks like that takes a while. |
|
Thanks @phiggins! |
This was done to support rspec/rspec-mocks#434 and depends on rspec/rspec-support#36.
I'm not extremely happy with the way I did the
fail_withtests that call the differ in support, but I wasn't sure if testing that the proper options were passed vs testing the actual integration with the differ was preferred: https://github.com/phiggins/rspec-expectations/blob/a3c1888b92b82456d166b9c39255f605a8261d2f/spec/rspec/expectations/fail_with_spec.rb