Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

assert: respect assert.doesNotThrow message.#6470

Closed
diversario wants to merge 2 commits intonodejs:masterfrom
diversario:i6400-assert-doesnotthrow
Closed

assert: respect assert.doesNotThrow message.#6470
diversario wants to merge 2 commits intonodejs:masterfrom
diversario:i6400-assert-doesnotthrow

Conversation

@diversario
Copy link
Copy Markdown

Addresses #6400.
Special handling to detect when user has supplied a custom message.
Added a test for user message.

Addresses nodejs#6400.
Special handling to detect when user has supplied a custom message.
Added a test for user message.
lib/assert.js Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's probably better to use util.isError(actual) here, especially after the changes made in 00eb8d4 based on #6352.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call.

When testing if `actual` value is an error use
`util.isError` instead of `instanceof`.
@daviddias
Copy link
Copy Markdown

@tjfontaine had you had the opportunity to look at this one? Seems good? :)

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Aug 13, 2015

@diversario ... unfortunately it doesn't look like this was ever reviewed further. Given that this involves a change in functionality, it would likely be best to close this PR here and open a new one against the active dev channel at https://github.com/nodejs/node if this is something you'd still like to pursue.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Aug 16, 2015

New issue in nodejs/node created to track this. The change would need to be made there and potentially cherry picked to v0.12 or v0.10. Closing this here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants