FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: handle `throw undefined` in assert.throws() by bnoordhuis · Pull Request #18029 · nodejs/node · GitHub

/ node Public

lib: handle throw undefined in assert.throws() - #18029

Closed
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix18027
ConversationCommits (1)ChecksFiles changed
Closed

lib: handle throw undefined in assert.throws()#18029
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix18027

Conversation

bnoordhuis commented Jan 7, 2018

Copy link
Copy Markdown
Member

And make assert.doesNotThrow() handle it as well.

Fixes: #18027

lib: handle throw undefined in assert.throws()
54f3cff 
And make `assert.doesNotThrow()` handle it as well.

Fixes: nodejs#18027
nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jan 7, 2018
bnoordhuis mentioned this pull request Jan 7, 2018
Closed
addaleax approved these changes Jan 7, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Maybe start the commit message with assert:? :)

BridgeAR approved these changes Jan 7, 2018
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but since this was never documented and never worked before, we might want to add a entry in the changed history part in the documentation.

TimothyGu approved these changes Jan 8, 2018
View reviewed changes

TimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 on a changed entry.

Comment thread lib/assert.js

const assert = module.exports = ok;

const NO_EXCEPTION_SENTINEL = {};

TimothyGu Jan 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would a symbol work here as well?

bnoordhuis Jan 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's merged now but for posterity: yes, but it doesn't add anything extra and it uses more memory than a plain empty object literal.

cjihrig approved these changes Jan 8, 2018
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. I like the idea of using a symbol for the sentinel.

jasnell approved these changes Jan 8, 2018
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but also prefer a Symbol for the sentinel

jasnell commented Jan 9, 2018

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/12470/

jasnell commented Jan 10, 2018

Copy link
Copy Markdown
Member

Landing this. Can switch the sentinel to a symbol in a separate PR later.

jasnell pushed a commit that referenced this pull request Jan 10, 2018
lib: handle throw undefined in assert.throws()
71203f5 
And make `assert.doesNotThrow()` handle it as well.

PR-URL: #18029
Fixes: #18027
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Jan 10, 2018

Copy link
Copy Markdown
Member

Landed in 71203f5

jasnell closed this Jan 10, 2018
bnoordhuis deleted the fix18027 branch January 10, 2018 09:06

evanlucas commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

This is not cherry-picking cleanly to v9.x-staging. If you would like for it to land there, please submit a backport pr. Thanks!

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
lib: handle throw undefined in assert.throws()
49dc778 
And make `assert.doesNotThrow()` handle it as well.

PR-URL: nodejs#18029
Fixes: nodejs#18027
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Mar 8, 2018
Closed
4 tasks

BridgeAR commented Mar 8, 2018

Copy link
Copy Markdown
Member

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
lib: handle throw undefined in assert.throws()
2f76179 
And make `assert.doesNotThrow()` handle it as well.

Backport-PR-URL: #19230
PR-URL: #18029
Fixes: #18027
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
lib: handle throw undefined in assert.throws()
b35eabb 
And make `assert.doesNotThrow()` handle it as well.

Backport-PR-URL: #19230
PR-URL: #18029
Fixes: #18027
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins commented Jul 31, 2018

Copy link
Copy Markdown
Contributor

Requested backport to 8.x in #19230

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
danbev danbev approved these changes
TimothyGu TimothyGu approved these changes
XadillaX XadillaX approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL