docs: rewrite `.throw` jsdoc #866

Merged
merged 1 commit into from Nov 15, 2016

Conversation

Projects
None yet
5 participants
@meeber
Contributor

meeber commented Nov 15, 2016

Closes #864

lib/chai/core/assertions.js
+ * instance.
+ *
+ * var err = new TypeError("Illegal salmon!");
+ * var badFn = function () { throw err };

This comment has been minimized.

@shvaikalesh

shvaikalesh Nov 15, 2016

Member

Nitpick note: throw err <-- semicolon here

@shvaikalesh

shvaikalesh Nov 15, 2016

Member

Nitpick note: throw err <-- semicolon here

@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Nov 15, 2016

Member

Fantastic job, @meeber. Very well written 👍

Member

shvaikalesh commented Nov 15, 2016

Fantastic job, @meeber. Very well written 👍

@vieiralucas

This comment has been minimized.

Show comment
Hide comment
@vieiralucas

vieiralucas Nov 15, 2016

Member

Thank you @meeber. This is really awesome work!

Just for the sake of discussion, this made me realize that this assertion actually checks if anything is thrown not only errors.

function stringThrower() {
   throw 'this is a string';
}

expect(stringThrower).to.throw();

IMHO this is an ok behavior and the docs can remain as it is.

What do you think?

Member

vieiralucas commented Nov 15, 2016

Thank you @meeber. This is really awesome work!

Just for the sake of discussion, this made me realize that this assertion actually checks if anything is thrown not only errors.

function stringThrower() {
   throw 'this is a string';
}

expect(stringThrower).to.throw();

IMHO this is an ok behavior and the docs can remain as it is.

What do you think?

lib/chai/core/assertions.js
+ * asserting that the thrown error is an instance of the given error
+ * constructor, or that the thrown error is referentially equal to the given
+ * error instance, and/or that the thrown error's message contains a given
+ * string or matches a given regular expression.

This comment has been minimized.

@keithamus

keithamus Nov 15, 2016

Member

This is quite a chunky sentence, I'd like to simplify this into a series of shorter statements, maybe even bullets? Maybe something like...

Invokes the target function, expecting it to throw an error. Passing in an Error constructor as the first argument will assert that the thrown error should match. Passing in an Error instance (new Error('...')) will assert that both errors should be referentially equal (===). Passing a string will assert that the thrown error's message matches the given string. Passing a regular expression will assert that the thrown error's message matches the given regular expression.

Then again, maybe we could simplify this whole paragraph much more given the examples below:

Invokes the target function, expecting it to throw an error. Providing arguments can be used to add extra assertions about the thrown error. See below for examples:

@keithamus

keithamus Nov 15, 2016

Member

This is quite a chunky sentence, I'd like to simplify this into a series of shorter statements, maybe even bullets? Maybe something like...

Invokes the target function, expecting it to throw an error. Passing in an Error constructor as the first argument will assert that the thrown error should match. Passing in an Error instance (new Error('...')) will assert that both errors should be referentially equal (===). Passing a string will assert that the thrown error's message matches the given string. Passing a regular expression will assert that the thrown error's message matches the given regular expression.

Then again, maybe we could simplify this whole paragraph much more given the examples below:

Invokes the target function, expecting it to throw an error. Providing arguments can be used to add extra assertions about the thrown error. See below for examples:

This comment has been minimized.

lib/chai/core/assertions.js
+ * expect(badFn).to.not.throw(ReferenceError);
+ *
+ * If the first argument is an error instance, `throw` invokes the function
+ * and asserts that an error is thrown that's referentially equal to that

This comment has been minimized.

@keithamus

keithamus Nov 15, 2016

Member

We should say here:

referentially equal (===)

@keithamus

keithamus Nov 15, 2016

Member

We should say here:

referentially equal (===)

+ *
+ * expect(function () { cat.meow(); }).to.throw(); // Function expression
+ * expect(() => cat.meow()).to.throw(); // ES6 arrow function
+ * expect(cat.meow.bind(cat)).to.throw(); // Bind

This comment has been minimized.

@keithamus

keithamus Nov 15, 2016

Member

This is all great. Very clear. Kudos.

@keithamus

keithamus Nov 15, 2016

Member

This is all great. Very clear. Kudos.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Nov 15, 2016

Member

Couple of requested changes @meeber - mostly around simplifying the wording, or making sure when we use terms like referentially equal we explain what that is. I want to be very very careful about our documentation - that it remains very approachable and we don't need users to learn new concepts or stumble over unknown words without reasonable redundancy in the explanation. Phrases like "referentially equal", while of course are common parlance for experienced devs, can quickly fall apart when explained to less experienced devs (even though they're aware of ===). Hopefully y'all agree 😄

Member

keithamus commented Nov 15, 2016

Couple of requested changes @meeber - mostly around simplifying the wording, or making sure when we use terms like referentially equal we explain what that is. I want to be very very careful about our documentation - that it remains very approachable and we don't need users to learn new concepts or stumble over unknown words without reasonable redundancy in the explanation. Phrases like "referentially equal", while of course are common parlance for experienced devs, can quickly fall apart when explained to less experienced devs (even though they're aware of ===). Hopefully y'all agree 😄

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Nov 15, 2016

Member

Awesome job! I totally agree with the changes @keithamus suggested, specially the first one. Shorter sentences could satisfy people's need to use this without having to read many details.

Thanks @meeber! 😄

Member

lucasfcosta commented Nov 15, 2016

Awesome job! I totally agree with the changes @keithamus suggested, specially the first one. Shorter sentences could satisfy people's need to use this without having to read many details.

Thanks @meeber! 😄

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Nov 15, 2016

Contributor

@keithamus @lucasfcosta Agreed on the first paragraph and the ===. Fixed both.

@vieiralucas Great point. Actually, I think it's worth mentioning how throw handles non-Errors, while also noting that it's a bad practice. Therefore, I've added a new paragraph to the end. Thoughts?

Contributor

meeber commented Nov 15, 2016

@keithamus @lucasfcosta Agreed on the first paragraph and the ===. Fixed both.

@vieiralucas Great point. Actually, I think it's worth mentioning how throw handles non-Errors, while also noting that it's a bad practice. Therefore, I've added a new paragraph to the end. Thoughts?

@vieiralucas

This comment has been minimized.

Show comment
Hide comment
@vieiralucas

vieiralucas Nov 15, 2016

Member

Awesome! LGTM

Member

vieiralucas commented Nov 15, 2016

Awesome! LGTM

@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Nov 15, 2016

Member

LGTM, very much agreed on specifying handling non-errors.

Member

shvaikalesh commented Nov 15, 2016

LGTM, very much agreed on specifying handling non-errors.

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Nov 15, 2016

Member

LGTM too!
Awesome job.

Member

lucasfcosta commented Nov 15, 2016

LGTM too!
Awesome job.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Nov 15, 2016

Member

Let's merge this!

Member

keithamus commented Nov 15, 2016

Let's merge this!

@keithamus keithamus merged commit 0836c45 into chaijs:master Nov 15, 2016

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meeber meeber deleted the meeber:throw-doc branch Aug 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment