Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fix to WebMock::API#hash_including #332

Merged
merged 4 commits into from Nov 17, 2013

Conversation

Projects
None yet
2 participants
Contributor

steookk commented Nov 12, 2013

fixes #329

Contributor

steookk commented Nov 13, 2013

@bblimke I'm done with my pull request.
Please, have a look at lines 53 and 73 of this file:
[https://github.com/steookk/webmock/blob/a219c4fd27995bf5469aa2deb8e334d5ed671196/spec/unit/matchers/hash_including_matcher_spec.rb]
I think it makes sense but tell me your opinion. Thanks!

Owner

bblimke commented Nov 13, 2013

@steookk wow, nice improvement and awesome specs, cheers! I can see you spent some time on it

Regarding hash_including({}), to me, without knowing earlier how rspec matcher treats it,
it was more intuitive that it will match any hash. Basically I read it as "match a hash, that includes (not equals but includes) the same key values pairs as key value pairs in provided hash".

In the behaviour you implemented, there is no difference between {} and hash_including({})

I'm in favour of keeping the behaviour the same as rspec hash_including matcher.

Can you please explain, why do you think it should only match empty hash though?

Contributor

steookk commented Nov 13, 2013

thanks!

In the behaviour you implemented, there is no difference between {} and hash_including({})

yes that's true. But with RSpec behavior there is no difference between hash_including({}) and not defining :body => .... at all.
It would be a matter of choosing which redundancy is more intuitive, which brings to the second consideration.

it was more intuitive that it will match any hash. Basically I read it as "match a hash, that includes (not equals but includes) the same key values pairs as key value pairs in provided hash".

I was reading it as "match a hash that includes nothing, in other words an empty match". The point is the use of Enumerable#all? which "returns true if the block never returns false or nil." Clearly, when you do {}.all? { find_a_key } it doesn't look for anything, so it will never return false, so it will always be true. This is a wrong choice in my opinion: it should return true only when keys are found, because the aim was to find something and we should be coherent with the aim.

Anyways, either choice is fine.

Owner

bblimke commented Nov 13, 2013

Thank you for sharing your point of view. I understand the reasons now.

I would agree with "match a hash that includes nothing" if the method was called hash_equals or hash_matches,
but hash_including to me, is about inclusion. In algebra you could call it superset_of instead. Any set is a superset of an empty set.
To be honest, Enumerable#all? makes sense to me too, I wouldn't expect it to return false for an empty collection. It's very subjective whether the aim was to find something or not. I think both Enumerable#all?
and Rspecs hash_including implement algebraically correct solution.

If it's about intuition, then I'm still in favour of hash_including({}) matching any hash.

@bblimke bblimke added a commit that referenced this pull request Nov 17, 2013

@bblimke bblimke Merge pull request #332 from steookk/master
fix to WebMock::API#hash_including
d31f4c8

@bblimke bblimke merged commit d31f4c8 into bblimke:master Nov 17, 2013

1 check passed

default The Travis CI build passed
Details
Owner

bblimke commented Nov 17, 2013

Released in 1.16.0 Cheers

Contributor

steookk commented Nov 20, 2013

Sorry that you had to do that fix: I should have been quicker.
Thanks for explaining me very well your reasons, especially the algebra superset_of
Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment