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' throw on non-string primitives and functions #812

Merged
merged 5 commits into from
Oct 1, 2016
Merged

Make 'empty' throw on non-string primitives and functions #812

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
89b9865
Make 'empty' throw on non-string primitives and functions
shvaikalesh Sep 30, 2016
ccc00bc
Update jsdoc
shvaikalesh Oct 1, 2016
92a40d4
improve error messages
shvaikalesh Oct 1, 2016
79e6fba
fix symbol to string coercion for Node 0.12
shvaikalesh Oct 1, 2016
a1c1dc4
use utils.inspect
shvaikalesh Oct 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions lib/chai/core/assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,8 @@ module.exports = function (chai, _) {
* ### .empty
*
* Asserts that the target's length is `0`. For arrays and strings, it checks
* the `length` property. For objects, it gets the count of
* enumerable keys.
* the `length` property. For non-function objects, it gets the count of own
* enumerable string keys.
*
* expect([]).to.be.empty;
* expect('').to.be.empty;
Expand All @@ -528,10 +528,26 @@ module.exports = function (chai, _) {
*/

Assertion.addProperty('empty', function () {
var obj = flag(this, 'object');
new Assertion(obj).to.exist;
var val = flag(this, 'object')
, itemsCount;

switch (_.type(val)) {
case 'array':
case 'string':
itemsCount = val.length;
break;
case 'function':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should handle generators too (because of recent updates to type-detect) and async function in the future.

var name = val.name ? ' ' + val.name : '';
throw new TypeError('.empty was passed a function' + name);
default:
if (val !== Object(val)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other option is to add case clause for other primitives' types.

Copy link
Member

Choose a reason for hiding this comment

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

Great use of Object(). 😍

Copy link
Member

Choose a reason for hiding this comment

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

That blew my mind to!

throw new TypeError('.empty was passed non-string primitive ' + String(val));
Copy link
Contributor Author

@shvaikalesh shvaikalesh Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

We need String here because it special-cases symbols. Otherwise, they throw on ToString.

EDIT: oops, that fails in Node 0.12 because of partial implementation. Let's do toString.

Copy link
Member

Choose a reason for hiding this comment

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

You could use the inspect utility.

Copy link
Member

@lucasfcosta lucasfcosta Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think I'm okay without it since the error message is pretty self-explanatory.
Also, if you want to add that anyway, I wouldn't mind, just try using _.type(), since it may be more accurate.

Copy link
Member

@lucasfcosta lucasfcosta Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I just noticed you want to print the value of what was passed, not it's type.
Then @vieiralucas is right, the inspect utility would be ideal.

Copy link
Member

@vieiralucas vieiralucas Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

_.type() is better IMO

Copy link
Member

@lucasfcosta lucasfcosta Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

okay, so, if we're going to use type we need to rephrase the error message to something like:

.empty was passed non-string primitive of type ' + _.type(val)

PS.: Sorry for all this mess in the comments @shvaikalesh 😆

Copy link
Member

@lucasfcosta lucasfcosta Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, calm down everyone hahahha:

THUMBS UP IF YOU VOTE FOR TYPE AND HEART THIS COMMENT IF YOU VOTE FOR INSPECT.
If we three vote there's no way this is going to be a tie.

Copy link
Contributor Author

@shvaikalesh shvaikalesh Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, let's do type then. Or not. Huh, need to refresh the page to see new comments, like in good ol' times.

Copy link
Contributor Author

@shvaikalesh shvaikalesh Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I am for inspect because you can definitely tell primitive type by the value, but not otherwise. May save console.log typing for someone.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so we're going for inspect.
Also, since inspect also adds delimiters which denote the type of the value (like [] for arrays and '' for strings), it is more complete.
Thanks for this @shvaikalesh!

LGTM!

}
itemsCount = Object.keys(val).length;
}

this.assert(
Object.keys(Object(obj)).length === 0
0 === itemsCount
Copy link
Contributor Author

@shvaikalesh shvaikalesh Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any ESLint config we can add to the project? This will greatly improve contribution/review experience: yoda comparisons and comma-first are not quite popular and it would be awesome to automate codestyle checks.

Copy link
Member

Choose a reason for hiding this comment

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

@shvaikalesh Yes, we thought about that too, we already discussed that here and we came to the conclusion that by moving all our code to separate repos with their own linting rules would be better, however, if you disagree you're welcome to share your thoughts with us 😄 .

, 'expected #{this} to be empty'
, 'expected #{this} not to be empty'
);
Expand Down
47 changes: 41 additions & 6 deletions test/expect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,28 +680,63 @@ describe('expect', function () {

err(function(){
expect(null).to.be.empty;
}, "expected null to exist");
}, ".empty was passed non-string primitive null");

err(function(){
expect(undefined).to.be.empty;
}, "expected undefined to exist");
}, ".empty was passed non-string primitive undefined");

err(function(){
expect().to.be.empty;
}, "expected undefined to exist");
}, ".empty was passed non-string primitive undefined");

err(function(){
expect(null).to.not.be.empty;
}, "expected null to exist");
}, ".empty was passed non-string primitive null");

err(function(){
expect(undefined).to.not.be.empty;
}, "expected undefined to exist");
}, ".empty was passed non-string primitive undefined");

err(function(){
expect().to.not.be.empty;
}, "expected undefined to exist");
}, ".empty was passed non-string primitive undefined");

err(function(){
expect(0).to.be.empty;
}, ".empty was passed non-string primitive 0");

err(function(){
expect(1).to.be.empty;
}, ".empty was passed non-string primitive 1");

err(function(){
expect(true).to.be.empty;
}, ".empty was passed non-string primitive true");

err(function(){
expect(false).to.be.empty;
}, ".empty was passed non-string primitive false");

if (typeof Symbol !== 'undefined') {
err(function(){
expect(Symbol()).to.be.empty;
}, ".empty was passed non-string primitive Symbol()");

err(function(){
expect(Symbol.iterator).to.be.empty;
}, ".empty was passed non-string primitive Symbol(Symbol.iterator)");
}

err(function(){
expect(function() {}).to.be.empty;
}, ".empty was passed a function");

if (FakeArgs.name === 'FakeArgs') {
err(function(){
expect(FakeArgs).to.be.empty;
}, ".empty was passed a function FakeArgs");
}
});

it('NaN', function() {
Expand Down
36 changes: 36 additions & 0 deletions test/should.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,42 @@ describe('should', function() {
err(function(){
({foo: 'bar'}).should.be.empty;
}, "expected { foo: \'bar\' } to be empty");

err(function(){
(0).should.be.empty;
}, ".empty was passed non-string primitive 0");

err(function(){
(1).should.be.empty;
}, ".empty was passed non-string primitive 1");

err(function(){
true.should.be.empty;
}, ".empty was passed non-string primitive true");

err(function(){
false.should.be.empty;
}, ".empty was passed non-string primitive false");

if (typeof Symbol !== 'undefined') {
err(function(){
Symbol().should.be.empty;
}, ".empty was passed non-string primitive Symbol()");

err(function(){
Symbol.iterator.should.to.be.empty;
}, ".empty was passed non-string primitive Symbol(Symbol.iterator)");
}

err(function(){
(function() {}).should.be.empty;
}, ".empty was passed a function");

if (FakeArgs.name === 'FakeArgs') {
err(function(){
FakeArgs.should.be.empty;
}, ".empty was passed a function FakeArgs");
}
});

it('finite(value)', function() {
Expand Down