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

shvaikalesh · 2016-09-30T20:26:32Z

Follow-up of #763.

shvaikalesh · 2016-09-30T20:30:23Z

lib/chai/core/assertions.js

    this.assert(
-        Object.keys(Object(obj)).length === 0
+        0 === itemsCount


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.

@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 😄 .

shvaikalesh · 2016-09-30T20:32:35Z

lib/chai/core/assertions.js

+      case 'function':
+        throw new TypeError('.empty() was passed a function');
+      default:
+        if (val !== Object(val)) {


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

shvaikalesh · 2016-09-30T20:33:16Z

lib/chai/core/assertions.js

+      case 'string':
+        itemsCount = val.length;
+        break;
+      case 'function':


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

meeber · 2016-10-01T00:21:29Z

@shvaikalesh Thank you for your continued work on this! Implementation looks fine to me. However, I think we should update the jsdoc to clarify that only non-function objects are allowed.

Other thoughts:

Agreed on ESLint. I'll start a new discussion on this after v4 is released.
You mentioned previously that you'll be following up this PR with another one that adds support for Map and Set. At that time, I'm wondering if we should begin throwing a TypeError if a WeakMap or WeakSet is passed. It seems to me that it'd likely be a programmer error to attempt to check if one of those two were empty without realizing that you can't do that.
It looks like we'll need a 3rd PR to add empty to the assert interface. Lemme know if you're interested in tackling that as well. Otherwise, I'll open a new issue for it.

shvaikalesh · 2016-10-01T08:06:06Z

@meeber I have updated jsdoc. Throwing on weak collections is nice idea, absolutely agree. Totally interested in adding empty to assert interface.

One more thought: it seems like tests for most assertions are repeated for every interface they are available for. Would be great to have only one test suite for each assertion.

meeber · 2016-10-01T12:24:00Z

@shvaikalesh Yeah adding tests is painful right now due to the multiple interfaces. There was a discussion about this somewhere but I can't find it. Anyway, the long term idea as we move into a modular system as described in #457 is to only have to write tests once for each assertion. But we're not quite there yet.

Regarding this PR: LGTM!

vieiralucas

Hello @shvaikalesh,

First of all thank you so much for all the effort you've been putting into this issue with .empty since #763.

I just add a comment about the messages inside TypeError

vieiralucas · 2016-10-01T17:34:10Z

lib/chai/core/assertions.js

+        itemsCount = val.length;
+        break;
+      case 'function':
+        throw new TypeError('.empty() was passed a function');


What about replacing .empty() with .empty since it is a property assertion?

vieiralucas · 2016-10-01T17:35:02Z

lib/chai/core/assertions.js

+        throw new TypeError('.empty() was passed a function');
+      default:
+        if (val !== Object(val)) {
+          throw new TypeError('.empty() was passed non-string primitive');


What about replacing .empty() with .empty since it is a property assertion?

vieiralucas · 2016-10-01T18:01:13Z

I'll just leave my LGTM in case you guys think that my comment is just nitpicking.

Agreed with you and @meeber about Map, Set, WekMap and WeakSet.
Agreed about adding empty to the assert interface.
Agreed about the problem with code style and missing eslint thing. 😸

P.S. This just made me realize that we might consider throwing TypeError instead of plain Error on closeTo, above, least and below.

lucasfcosta

Thanks for your work @shvaikalesh, this is looking great, however, as @vieiralucas said maybe we could just swap .empty() for .empty in the error messages.
After that I will be very happy to merge this in.
Thanks! 😄

lucasfcosta · 2016-10-01T17:56:52Z

lib/chai/core/assertions.js

+      case 'function':
+        throw new TypeError('.empty() was passed a function');
+      default:
+        if (val !== Object(val)) {


Great use of Object(). 😍

That blew my mind to!

lucasfcosta · 2016-10-01T17:59:56Z

lib/chai/core/assertions.js

    this.assert(
-        Object.keys(Object(obj)).length === 0
+        0 === itemsCount


@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 😄 .

lucasfcosta · 2016-10-01T18:01:58Z

lib/chai/core/assertions.js

+        itemsCount = val.length;
+        break;
+      case 'function':
+        throw new TypeError('.empty() was passed a function');


As @vieiralucas said, I think .empty() should be changed to .empty since it's a property assertion.

meeber · 2016-10-01T18:06:23Z

@lucasfcosta @vieiralucas UH OH THE LUCAS SYNDICATE IS IN FULL FORCE TODAY. Just a reminder to whoever ends up merging: Squash and merge! :D

lucasfcosta · 2016-10-01T18:08:43Z

@meeber hahaha yes, thanks for remembering us to do that 😄
Let's just wait for the .empty() change to make the error more self-explanatory (which IMO is a very important thing). Are you guys okay with that or am I being too nitpicky?

shvaikalesh · 2016-10-01T18:30:48Z

I have removed () and added function names and primitives values to messages for better debugging experience (is that OK?).

shvaikalesh · 2016-10-01T18:31:47Z

lib/chai/core/assertions.js

      default:
        if (val !== Object(val)) {
-          throw new TypeError('.empty() was passed non-string primitive');
+          throw new TypeError('.empty was passed non-string primitive ' + String(val));


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.

You could use the inspect utility.

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.

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.

_.type() is better IMO

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 😆

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.

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

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

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!

vieiralucas · 2016-10-01T18:57:41Z

LGTM again because something went wrong with approvals/lgtm

shvaikalesh · 2016-10-01T18:59:25Z

Thanks for the merge, awesome contribution experience ❤️ .

lucasfcosta · 2016-10-01T18:59:26Z

Squashed and merged!
Awesome work @shvaikalesh, I'm looking forward to see you here again 😄

vieiralucas · 2016-10-01T19:01:32Z

Thank you so much @shvaikalesh, looking forward for your next contributions. 😄

meeber · 2016-10-01T19:06:07Z

No more caffeine for The Lucas Syndicate.

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

89b9865

shvaikalesh commented Sep 30, 2016

View reviewed changes

Update jsdoc

ccc00bc

vieiralucas reviewed Oct 1, 2016

View reviewed changes

lucasfcosta requested changes Oct 1, 2016

View reviewed changes

improve error messages

92a40d4

shvaikalesh commented Oct 1, 2016

View reviewed changes

shvaikalesh added 2 commits October 1, 2016 21:39

fix symbol to string coercion for Node 0.12

79e6fba

use utils.inspect

a1c1dc4

lucasfcosta approved these changes Oct 1, 2016

View reviewed changes

lucasfcosta merged commit 5b170b2 into chaijs:master Oct 1, 2016

shvaikalesh mentioned this pull request Oct 2, 2016

Make empty assertion work with es6 collections #814

Merged

lucasfcosta mentioned this pull request Oct 2, 2016

4.x.x Complete Migration Guide #781

Closed

shvaikalesh mentioned this pull request Oct 5, 2016

Add .empty to assert interface #828

Merged

lucasfcosta mentioned this pull request Nov 2, 2016

Drop support of node v0.10 #816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shvaikalesh commented Sep 30, 2016

shvaikalesh Sep 30, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016

shvaikalesh Sep 30, 2016

shvaikalesh Sep 30, 2016

meeber commented Oct 1, 2016

shvaikalesh commented Oct 1, 2016 •

edited

Loading

meeber commented Oct 1, 2016

vieiralucas left a comment

vieiralucas Oct 1, 2016 •

edited

Loading

vieiralucas Oct 1, 2016 •

edited

Loading

vieiralucas commented Oct 1, 2016 •

edited

Loading

lucasfcosta left a comment

lucasfcosta Oct 1, 2016

vieiralucas Oct 1, 2016

lucasfcosta Oct 1, 2016

lucasfcosta Oct 1, 2016

meeber commented Oct 1, 2016

lucasfcosta commented Oct 1, 2016

shvaikalesh commented Oct 1, 2016

shvaikalesh Oct 1, 2016 •

edited

Loading

vieiralucas Oct 1, 2016

lucasfcosta Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

vieiralucas Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

shvaikalesh Oct 1, 2016 •

edited

Loading

shvaikalesh Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016

vieiralucas commented Oct 1, 2016 •

edited

Loading

shvaikalesh commented Oct 1, 2016

lucasfcosta commented Oct 1, 2016

vieiralucas commented Oct 1, 2016

meeber commented Oct 1, 2016

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

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

Conversation

shvaikalesh commented Sep 30, 2016

shvaikalesh Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber commented Oct 1, 2016

shvaikalesh commented Oct 1, 2016 • edited Loading

meeber commented Oct 1, 2016

vieiralucas left a comment

Choose a reason for hiding this comment

vieiralucas Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

vieiralucas Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

vieiralucas commented Oct 1, 2016 • edited Loading

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber commented Oct 1, 2016

lucasfcosta commented Oct 1, 2016

shvaikalesh commented Oct 1, 2016

shvaikalesh Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

lucasfcosta Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

vieiralucas Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

lucasfcosta Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

lucasfcosta Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

shvaikalesh Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

shvaikalesh Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vieiralucas commented Oct 1, 2016 • edited Loading

shvaikalesh commented Oct 1, 2016

lucasfcosta commented Oct 1, 2016

vieiralucas commented Oct 1, 2016

meeber commented Oct 1, 2016

shvaikalesh Sep 30, 2016 •

edited

Loading

shvaikalesh commented Oct 1, 2016 •

edited

Loading

vieiralucas Oct 1, 2016 •

edited

Loading

vieiralucas Oct 1, 2016 •

edited

Loading

vieiralucas commented Oct 1, 2016 •

edited

Loading

shvaikalesh Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

vieiralucas Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

lucasfcosta Oct 1, 2016 •

edited

Loading

shvaikalesh Oct 1, 2016 •

edited

Loading

shvaikalesh Oct 1, 2016 •

edited

Loading

vieiralucas commented Oct 1, 2016 •

edited

Loading