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

Add toIncludeKeys and toExcludeKeys #87

Merged
merged 7 commits into from
Apr 18, 2016

Conversation

calebmer
Copy link
Contributor

@calebmer calebmer commented Apr 9, 2016

This was a feature I originally included in #86, but was asked to move it to its own PR. Therefore, tada 🎉

This PR currently adds two methods to Expectation, toIncludeKeys and toExcludeKeys which work as expected.

@@ -286,6 +286,32 @@ expect('hello world').toExclude('goodbye')
expect('hello world').toNotContain('goodbye')
```

### toIncludeKeys
> `expect(object).toIncludeKeys(keys, [hasKey], [message])`<br>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand what the hasKey argument is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with other methods, also someone might want to use a check which includes properties in the prototype chain. The has module only checks immediate properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe toInclude and toExclude only deal with own properties - I think we should be consistent here. In addition, "own" should be the default, I'd think.

I'd also want a separate method to deal with inherited properties, rather than a boolean switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a boolean switch, it's an optional comparator function like toInclude has 😊

I just updated the docs, reread and if you're still opposed I can take it out. I only added the parameter for some consistency with the comparater function in toInclude.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah - in that case let's make it a comparator argument, just the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😊 👍

@@ -364,6 +365,42 @@ class Expectation {
return this
}

toIncludeKeys(keys, hasKey, message) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's call hasKey comparator, for consistency with the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@wuct
Copy link
Contributor

wuct commented Apr 14, 2016

Thanks for this PR! These features are really useful. Actually, I have implemented something similar in my projects as a plugin, but your code is more robust :)

P.S. There is a small typo in the first comment. The original pr should be #86 not #85.

@@ -286,6 +286,32 @@ expect('hello world').toExclude('goodbye')
expect('hello world').toNotContain('goodbye')
```

### toIncludeKeys
Copy link
Owner

Choose a reason for hiding this comment

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

Let's write this as toIncludeKey(s)

@mjackson mjackson self-assigned this Apr 18, 2016
@mjackson
Copy link
Owner

This looks great. Thanks, @calebmer! I made a few comments that I hope will help.

@calebmer
Copy link
Contributor Author

👍 Updated this PR and #86. I want to complement again core contributors on how clean this code base is. Jumping into it is very fun and easy.

@mjackson mjackson merged commit ee939ab into mjackson:master Apr 18, 2016
@mjackson
Copy link
Owner

Thanks, @calebmer :)

@calebmer calebmer deleted the feat/object-include-key branch April 18, 2016 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants