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

Keys for maps and sets #633

Merged
merged 3 commits into from
Mar 27, 2016
Merged

Conversation

lucasfcosta
Copy link
Member

Hello everyone, this PR aims to solve #632.

Here's a summary of the changes I made:

  1. I am using .some() instead of .filter() when checking for any keys. There is no need to calculate the full size of intersections (intersection > 0), if we have one it's already bigger than 0 therefore we don't need to iterate through everything.
  2. I had to change the mixedArgs message because keys assertion now accepts objects as keys (as I've said on Add assertions for ES6 Maps and Sets #632).
  3. When checking maps and sets' keys we need to compare objects, that's why I'm using _.eql(expectedKey, actualKey).
  4. I noticed that the assert interface had no keys related methods, so I added every possible combination (any/all/contains/have - Please notice that some of these combinations have the same meaning, as explained on the keys method description, so I added only the ones with different effects)
  5. I included tests for these changes on all three interface's test files.
  6. Every method I've touched got it's JSDoc comments updated (examples were included too).

PS.: When creating these tests I've had to use if (Maps !== undefined) and if (Sets !== undefined) because apparently PhantomJS 1.9 uses an engine that hasn't implemented these specs yet.

Feel free to point possible mistakes and give feedback or new ideas.

Thanks everyone

@@ -1142,6 +1145,9 @@ module.exports = function (chai, _) {
* expect({ foo: 1, bar: 2 }).to.have.all.keys({'bar': 6, 'foo': 7});
* expect({ foo: 1, bar: 2, baz: 3 }).to.contain.all.keys(['bar', 'foo']);
* expect({ foo: 1, bar: 2, baz: 3 }).to.contain.all.keys({'bar': 6});
* expect(new Map([[{objKey: 'value'}, 'value'], [1, 2]])).to.contain.key({objKey: 'value'});
* expect(new Map([['firstKey', 'firstValue'], [1, 2]])).to.contain.all.keys('firstKey', 1);
* expect(new Set([['foo', 'bar'], ['example', 1]])).to.have.any.keys('foo');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have an example of passing multiple objects as keys here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be good!
Totally forgot it, thanks for pointing this

@keithamus
Copy link
Member

Only a couple of minor points above. Otherwise this looks awesome @lucasfcosta. Great work, as usual 😄

@lucasfcosta
Copy link
Member Author

Thanks, @keithamus!
Your feedback is great as usual too 😄

Well, I'm aware _.eql uses deep equality and I've used it because when, for example, a Map has an object as Key and I compare the objects (even if they're equal) I will get false as result because === checks for referential equality, therefore this, for example, wouldn't work:

expect(new Map([[{objKey: 'value'}, 'value'], [1, 2]])).to.contain.key([{objKey: 'value'}]);

This will fail, because although the object used as key on the map is depply equal to the one passed to the .key assertion they're not the same so === will return false. If you want to check for referential equality only then I feel like we should use ===.

@keithamus
Copy link
Member

@lucasfcosta sure, I expected you to say as much - I should have elaborated more in my original comment, but got a little side tracked with other things.

Deep equality is going to be sometimes desirable within Maps, but sometimes referential equality will be desirable. If we start with using referential equality, we can always move to deep equality later - by adding the .deep qualifier. We won't be able to (easily) do the same the other way around.

@lucasfcosta
Copy link
Member Author

Ah, I understand now and I totally agree, it really seems to be the right step. I haven't thought about it before.
I can also implement the deep check when there is a deep flag if you want me to.

Thanks for the explanation, I'll make sure to update this PR with the changes we talked about ASAP 😃

@keithamus
Copy link
Member

We can tackle the deep flag in another PR, to keep this one simple 😄

@lucasfcosta lucasfcosta force-pushed the keys-for-maps-and-sets branch 6 times, most recently from c675734 to 22a8531 Compare March 9, 2016 22:46
@lucasfcosta lucasfcosta force-pushed the keys-for-maps-and-sets branch from 22a8531 to c654360 Compare March 9, 2016 22:48
@lucasfcosta
Copy link
Member Author

This one is done! 🎉
I look forward to implementing the deep flag support on another PR later, if you don't mind. I feel like it would be very useful when it comes to Maps and Sets.

And thanks again for your support!

@sukrosono
Copy link
Contributor

@lucasfcosta awesome 😄

@keithamus keithamus merged commit bf1e0bf into chaijs:master Mar 27, 2016
@keithamus
Copy link
Member

Fantastic work as usual @lucasfcosta. Feel free to make a PR adding support for deep Map/Set keys 😄

@lucasfcosta
Copy link
Member Author

Thanks @keithamus, I'll certainly do it 😄

@sukrosono
Copy link
Contributor

close #632 ?

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

Successfully merging this pull request may close these issues.

3 participants