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.

Sign up for GitHub

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

Jump to bottom

Make empty assertion work with es6 collections #814

Merged
merged 4 commits into from
Oct 2, 2016
Merged

Make empty assertion work with es6 collections #814

merged 4 commits into from
Oct 2, 2016

Conversation

shvaikalesh
Copy link
Contributor

@shvaikalesh shvaikalesh commented Oct 2, 2016 •
edited
Loading

Follow-up of #763 and #812.

vieiralucas
vieiralucas requested changes Oct 2, 2016
View reviewed changes
lib/chai/core/assertions.js
break;
case 'weakmap':
case 'weakset':
throw new TypeError('.empty was called on a weak collection');
Copy link
Member

Choose a reason for hiding this comment

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

I think that to keep consistency with the TypeError messages added to #812, this should be something like this:
.empty was passed a weak collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks, fixed that. Another error message was just 3 lines away, what's wrong with me...

@vieiralucas
Copy link
Member

vieiralucas commented Oct 2, 2016 •
edited
Loading

Hello @shvaikalesh, I'm very happy to see you here again.

This change seems very clean, awesome work here.
I just made a comment on the message you added on weak collections. Let me know what you think, feel free to disagree with me.

EDIT: OMG YOU'RE SO FAST.

Well, this LGTM now

@shvaikalesh
Copy link
Contributor Author

shvaikalesh commented Oct 2, 2016 •
edited
Loading

Not that fast, forgot the tests :(

Should pass now.

@lucasfcosta
Copy link
Member

lucasfcosta commented Oct 2, 2016 •
edited
Loading

Well done, LGTM too!
Thanks so much for all these awesome contributions 😄
@vieiralucas would you like to do the honors and squash + merge this?

@meeber
Copy link
Contributor

meeber commented Oct 2, 2016

Sec let me check something

@meeber
Copy link
Contributor

meeber commented Oct 2, 2016

@shvaikalesh Annoyingly, Map and Set constructor arguments aren't supported in IE11 so will need to set the values manually in the tests. Also recommend adding a little note like this. Otherwise looks great!

@shvaikalesh
Copy link
Contributor Author

shvaikalesh commented Oct 2, 2016 •
edited
Loading

@meeber Nice catch, I kinda forgot about partial implementations. Too spoiled by shims 😄

@meeber
Copy link
Contributor

meeber commented Oct 2, 2016

LGTM! Thank you for your contributions! :D

@vieiralucas vieiralucas merged commit c38bfec into chaijs:master Oct 2, 2016
@vieiralucas
Copy link
Member

Squashed and merged!

Thank you again @shvaikalesh

@lucasfcosta lucasfcosta mentioned this pull request Oct 2, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vieiralucas vieiralucas vieiralucas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shvaikalesh @vieiralucas @lucasfcosta @meeber