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

assert: fix exception message for assert(0) on try catch block #46760

Merged
merged 4 commits into from
Feb 26, 2023
Merged

assert: fix exception message for assert(0) on try catch block #46760

merged 4 commits into from
Feb 26, 2023
+78 −19

Conversation

hidecology
Copy link
Contributor

Fixes: #30872

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Feb 22, 2023
BridgeAR
BridgeAR approved these changes Feb 22, 2023
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Great work! LGTM

lib/assert.js Outdated
@@ -242,39 +243,42 @@ function getCode(fd, line, column) {

function parseCode(code, offset) {
// Lazy load acorn.
if (parseExpressionAt === undefined) {
if (parseExpressionAt === undefined || tokenizer === undefined) {
Copy link
Member

@BridgeAR BridgeAR Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if (parseExpressionAt === undefined || tokenizer === undefined) {
if (parseExpressionAt === undefined) {

The second condition is not needed due to the tokenizer and parseExpressionAt being defined at the same moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BridgeAR
Thanks for your feedback.
I see. I will revert this conditional expression back to the previous one.

lib/assert.js Outdated
// expression is bigger than the provided buffer.
// eslint-disable-next-line no-throw-literal
throw null;
if (node && node.node.end >= offset) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (node && node.node.end >= offset) {
if (node?.node.end >= offset) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BridgeAR
Got it. I will change this code.

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2023
@nodejs-github-bot
Copy link
Collaborator

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

cola119
cola119 approved these changes Feb 24, 2023
View reviewed changes
test/parallel/test-assert.js Show resolved Hide resolved
test/parallel/test-assert.js Show resolved Hide resolved
test/parallel/test-assert.js Show resolved Hide resolved
@cola119 cola119 requested a review from aduh95 February 24, 2023 06:55
@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 26, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 26, 2023
@nodejs-github-bot nodejs-github-bot merged commit 3c0131a into nodejs:main Feb 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 3c0131a

targos pushed a commit that referenced this pull request Mar 13, 2023
@hidecology @targos
assert: fix exception message for assert(0) on try catch block
eaca288 
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
@hidecology @targos
assert: fix exception message for assert(0) on try catch block
e11f08e 
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
@targos targos mentioned this pull request Mar 14, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@hidecology @danielleadams
assert: fix exception message for assert(0) on try catch block
c742493 
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cola119 cola119 cola119 approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception message for assert(0) depends on whitespace
5 participants
@hidecology @nodejs-github-bot @BridgeAR @aduh95 @cola119