-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
expect(inList).to.be.oneOf
assertion
#534
Conversation
Adds a way to test if a value appears in a flat list. Compliments `include.members` to reverse the order of the input. Now the list appears in the assertion, as opposed to the `expect`. Closes #532
I'll push up a commit with a version bump once this gets slated for a merge into master (or you can just do it for me). |
var expected = flag(this, 'object'); | ||
new Assertion(list).to.be.an('array'); | ||
new Assertion(expected).to.not.be.an('array'); | ||
new Assertion(expected).to.not.be.an('object'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this requirement really matters? Would it be so wrong to have oneOf
work with object or arrays? Referential equality would be fine for an initial implementation - we can add .deep.oneOf()
later if we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could probably get that in there. Just so you know, it'll act like Python's in
keyword. Meaning:
>>> [3] in [1, 2, [3]]
True
>>> [4] in [1, 2, [3, [4]]]
False
>>> [3, [4]] in [1, 2, [3, [4]]]
True
In other words, unless I'm matching something from the first level all the way to the end of that first level object-type, it'll fail. That's the simplest way I can think to handle object-types inside of a list without a deep
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the restrictions, and without deep, I'd expect the following:
expect(3).to.be.oneOf([1,2,3]) // pass
var three = [3];
expect(three).to.be.oneOf([1,2,three]) // pass
expect([3]).to.be.oneOf([1,2,[3]]) // fails because [3] !== [3] (two different array literals)
The above is fine and completely expected and acceptable piece of JavaScript. We can add .deep
later to do more interesting things, but there's not reason to stop people doing the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll do that. I was using _.isEqual
(which is like ==
for objects in lodash), but now I will use strict referencing via ===
.
@Droogans don't worry about version bumping or anything - that's handled as part of the release process. |
@keithamus I'm not sure you caught this but I added object support to the PR. |
Sorry @Droogans, I did not. Github (annoyingly) only notifies when someone comments in a PR, not when they push up new changes. For future, I'd recommend any changes to a PR are followed up with a comment to bump to the relevant project, until Github gets their act together 😉 |
} | ||
}); | ||
return matched; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have this function in the previous commits did we? I'm not sure it is needed. indexOf
should work just as well here, no?
(from http://www.ecma-international.org/ecma-262/6.0/index.html#sec-array.prototype.indexof)
indexOf compares searchElement to the elements of the array, in ascending order, using the Strict Equality Comparison algorithm (7.2.13), and if found at one or more indices, returns the smallest such index; otherwise, −1 is returned.
I think having list.indexOf(expected) !== -1
would work just as well. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keithamus I put it back...I was halfway done with _.isEquals
-style object comparison (content matches, not references). I forgot that I could use the simpler indexOf
once you asked for reference-based matching.
I force pushed it up to keep the history tidy.
Wonderful work @Droogans! Also thank-you for keeping a really nice commit history - I was considering adding some contributing guidelines to enforce Angular style commit messages, and you may have just made a convincing argument with your lovely tidy commits. |
`expect(inList).to.be.oneOf` assertion
I see now why you wanted to name it
oneOf
, as opposed toin
. The rules for matching against arbitrarily deep objects using_.isEqual()
were not very clear. This PR looks to add just enough functionality to support most typical use cases.This will
reject anything in theallow for strict reference comparisons for all objects that are passed in. For deeply nested objects, only first level members are considered for matches.expect
part of the test that is an array or an object. It will only match vailist.indexOf(inList) > -1