-
-
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
Add ES6 collection support to include() #994
Changes from 5 commits
07de33e
86a9ea3
4d56c3d
0e809c3
defc699
039564d
df6badd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -449,25 +449,24 @@ module.exports = function (chai, _) { | |
* @api public | ||
*/ | ||
|
||
function includeChainingBehavior () { | ||
flag(this, 'contains', true); | ||
function SameValueZero(a, b) { | ||
return (_.isNaN(a) && _.isNaN(b)) || a === b; | ||
} | ||
|
||
function isDeepIncluded (arr, val) { | ||
return arr.some(function (arrVal) { | ||
return _.eql(arrVal, val); | ||
}); | ||
function includeChainingBehavior () { | ||
flag(this, 'contains', true); | ||
} | ||
|
||
function include (val, msg) { | ||
if (msg) flag(this, 'message', msg); | ||
|
||
_.expectTypes(this, ['array', 'object', 'string']); | ||
_.expectTypes(this, [ | ||
'array', 'object', 'string', | ||
'map', 'set', 'weakmap', 'weakset', | ||
]); | ||
|
||
var obj = flag(this, 'object') | ||
, objType = _.type(obj).toLowerCase() | ||
, isDeep = flag(this, 'deep') | ||
, descriptor = isDeep ? 'deep ' : ''; | ||
, objType = _.type(obj).toLowerCase(); | ||
|
||
// This block is for asserting a subset of properties in an object. | ||
if (objType === 'object') { | ||
|
@@ -504,10 +503,63 @@ module.exports = function (chai, _) { | |
return; | ||
} | ||
|
||
// Assert inclusion in an array or substring in a string. | ||
var isDeep = flag(this, 'deep') | ||
, descriptor = isDeep ? 'deep ' : '' | ||
, included = false; | ||
|
||
switch (objType) { | ||
case 'string': | ||
included = obj.indexOf(val) !== -1; | ||
break; | ||
|
||
case 'weakmap': | ||
case 'weakset': | ||
if (isDeep) { | ||
var flagMsg = flag(this, 'message') | ||
, ssfi = flag(this, 'ssfi'); | ||
flagMsg = flagMsg ? flagMsg + ': ' : ''; | ||
|
||
throw new AssertionError( | ||
flagMsg + 'unable to use .deep.include with weak collection', | ||
undefined, | ||
ssfi | ||
); | ||
} | ||
|
||
included = obj.has(val); | ||
break; | ||
|
||
case 'map': | ||
var isEql = isDeep ? _.eql : SameValueZero; | ||
obj.forEach(function (item) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As @vieiralucas said, we might be able to optimize this by getting out of the loop when we found the included item. In order to support IE and older versions of it we can This is a cross-platform compatible implementation I've made on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lucasfcosta I think this is worth exploring. I'd prefer this kind of optimization technique be implemented in a separate PR as a refactor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @meeber agreed. LGTM! Let's get this merged. |
||
included = included || isEql(item, val); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do something here to optimize this by breaking out the loop once I realize that then we would need to have 3 implementations, one for Do you think it is worth it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can optimize away looping through case 'map':
let isEql = isDeep ? _.eql : function(a, b) { return a === b; };
// I believe SameValueZero should be ^^ here to be on par with Set#has
obj.forEach(function (item) {
included = included || isEql(item, val);
});
break;
case 'set':
if (isDeep) {
obj.forEach(function (item) {
included = included || _.eql(item, val);
});
} else {
included = obj.has(val);
}
break;
case 'array':
if (isDeep) {
included = obj.some(function (item) {
return _.eql(item, val);
});
} else {
included = obj.indexOf(val) !== -1;
}
break; I am not sure the benefits outweigh code amount & complexity. EDIT: sorry, email readers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good but I'd be a little careful here with IE11 - which has terrible support for Map/Set. It probably will work but I've often been surprised how seemingly obvious features are missing from IE11's Map/Set |
||
break; | ||
|
||
case 'set': | ||
if (isDeep) { | ||
obj.forEach(function (item) { | ||
included = included || _.eql(item, val); | ||
}); | ||
} else { | ||
included = obj.has(val); | ||
} | ||
break; | ||
|
||
case 'array': | ||
if (isDeep) { | ||
included = obj.some(function (item) { | ||
return _.eql(item, val); | ||
}) | ||
} else { | ||
included = obj.indexOf(val) !== -1; | ||
} | ||
break; | ||
} | ||
|
||
// Assert inclusion in collection or substring in a string. | ||
this.assert( | ||
objType === 'string' || !isDeep ? ~obj.indexOf(val) | ||
: isDeepIncluded(obj, val) | ||
included | ||
, 'expected #{this} to ' + descriptor + 'include ' + _.inspect(val) | ||
, 'expected #{this} to not ' + descriptor + 'include ' + _.inspect(val)); | ||
} | ||
|
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 think we should consider containing code like this into a private method: it is easy to forget to process
flagMsg
, passssfi
, and skip second argument.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.
Agreed, but I'd like to see focus on #585 to get the assertion functions to be pure functions returning objects, so we can wrap them up in
AssertionErrors