-
Notifications
You must be signed in to change notification settings - Fork 55
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 rspec-mocks argument macthers #128
Add support for rspec-mocks argument macthers #128
Conversation
array = if SuperDiff::RSpec.a_collection_including_something?(value) | ||
value.expecteds | ||
else | ||
value.instance_variable_get(:@expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is a bad practice, but unfortunately, the rspec-mocks matcher didn't have a public interface to access the values.
https://github.com/rspec/rspec-mocks/blob/v3.10.2/lib/rspec/mocks/argument_matchers.rb#L231-L259
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! (I also do this in other places)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few little comments but overall this is a nice PR, thank you for doing this!
@@ -3,7 +3,8 @@ module RSpec | |||
module Differs | |||
class CollectionIncluding < SuperDiff::Differs::Array | |||
def self.applies_to?(expected, actual) | |||
SuperDiff::RSpec.a_collection_including_something?(expected) && | |||
(SuperDiff::RSpec.a_collection_including_something?(expected) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: What do you think about formatting it like this so the grouping is easier to parse?
(
SuperDiff::RSpec.a_collection_including_something?(expected) ||
SuperDiff::RSpec.array_including_something?(expected)
) && actual.is_a?(::Array)
or
(
SuperDiff::RSpec.a_collection_including_something?(expected) ||
SuperDiff::RSpec.array_including_something?(expected)
) &&
actual.is_a?(::Array)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array = if SuperDiff::RSpec.a_collection_including_something?(value) | ||
value.expecteds | ||
else | ||
value.instance_variable_get(:@expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! (I also do this in other places)
lib/super_diff/rspec/operation_tree_builders/collection_including.rb
Outdated
Show resolved
Hide resolved
c98b483
to
60938bd
Compare
This is great! Thanks so much for adding this! |
Now available in v0.7.0! |
This PR adds support for diffing rspec-mocks argument matchers. I often use
hash_including
in rspec-mocks instead ofa_hash_including
in rspec-expectations for partial hash comparisons. SuperDiff shows a great diff fora_hash_including
, buthash_including
doesn't.In rspec-mocks, there is an argument matcher similar to the matcher provided by rspec-expectations. This PR is primarily aimed at them. The following support has been added:
hash_including
(similar toa_hash_including
)array_including
(similar toa_collection_including
)kind_of
(similar toa_kind_of
)instance_of
(similar toan_instance_of
)Other than that, rspec-mocks provides useful matchers (e.g.
hash_excluding,
duck_type`, etc.), but they are out of scope here.Before
After
Finally, thank you for the great gem. In our project, we had a problem that the diff of the request spec comparing huge JSON was very hard to read, but this gem solved it brilliantly.