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 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/chai/core/assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,10 +537,11 @@ module.exports = function (chai, _) {
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.

throw new TypeError('.empty() was passed a function');
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');
throw new TypeError('.empty was passed non-string primitive ' + String(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.

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

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

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

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

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

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

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

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;
}
Expand Down
34 changes: 22 additions & 12 deletions test/expect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,53 +680,63 @@ describe('expect', function () {

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

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

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

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

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

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

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

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

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

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

if (typeof Symbol !== 'undefined') {
err(function(){
expect(Symbol()).to.be.empty;
}, ".empty() was passed non-string primitive");
}, ".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");
}, ".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
22 changes: 16 additions & 6 deletions test/should.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,29 +660,39 @@ describe('should', function() {

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

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

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

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

if (typeof Symbol !== 'undefined') {
err(function(){
Symbol().should.be.empty;
}, ".empty() was passed non-string primitive");
}, ".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");
}, ".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