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

Deep equality check for collection membership #228

Merged
merged 1 commit into from Jan 30, 2014

Conversation

@duncanbeevers
Copy link
Contributor

@duncanbeevers duncanbeevers commented Dec 27, 2013

Support deep equality comparison for collection membership.

expect([{ id: 1 }, { id: 2 }]).to.have.deep.members([{ id: 1 }, { id: 2 }])
@coveralls
Copy link

@coveralls coveralls commented Dec 27, 2013

Coverage Status

Coverage remained the same when pulling 476c636 on duncanbeevers:deep_members into 564af34 on chaijs:master.

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Jan 29, 2014

This PR fails on Travis. It seems to be due to a karma problem which we have fixed in master. Can you please rebase upstream?

Furthermore, please include a failing and a negation test case for your new assertion.
Finally, please review #230 and explain how your PR is different.

Thanks.

@duncanbeevers
Copy link
Contributor Author

@duncanbeevers duncanbeevers commented Jan 30, 2014

Rebased and added new test cases.

#230 applies to the include assertion, while this PR applies to the members assertion.

members is more strict than include, although the two can be used in conjunction to produce a weaker members (superset comparison vs equivalence comparison)

Additionally, this PR only adds the deep object comparison behavior when the deep flag is applied allowing for the stricter object-identity comparison behavior to continue being used.

For example, with this PR, the following test passes.

expect([{ id: 1 }]).deep.include.members([{ id: 1 }]); // passes

While this test does not.

expect([{ id: 1 }]).include.members([{ id: 1 }]); // fails
@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Jan 30, 2014

Ah fantastic! Thank you. One final thing... can you update the inline comment for members specifying that it can support deep and include your code samples.

I just release 1.9.0 not but an hour ago. Sorry this didn't make it in. I will be releasing 1.9.1 on Friday with a few non-critical things I couldn't get to this evening. Given your promptness it shouldn't be an issue getting this included as well.

@duncanbeevers
Copy link
Contributor Author

@duncanbeevers duncanbeevers commented Jan 30, 2014

I updated the inline documentation to include a deep.include example as well as added an additional test case to the non-deep members spec to highlight when the flag might be needed.

@duncanbeevers
Copy link
Contributor Author

@duncanbeevers duncanbeevers commented Jan 30, 2014

Additionally, I actually think #230 is a little over-reaching. I like having strict-equality comparison available and exposing the more computationally-expensive and conceptually-fuzzier deep-equality through assertion flags.

logicalparadox added a commit that referenced this pull request Jan 30, 2014
Deep equality check for collection membership
@logicalparadox logicalparadox merged commit 407ce59 into chaijs:master Jan 30, 2014
1 check passed
1 check passed
@logicalparadox
default The Travis CI build passed
Details
@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Jan 30, 2014

Looks great, merged. Also, fair point about #230. Will investigate and discuss.

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

Successfully merging this pull request may close these issues.

None yet

3 participants