Change error message to not defined by MohammedEssehemy · Pull Request #26857 · nodejs/node (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits1 Checks0 Files changed
Conversation
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]({{ revealButtonHref }})
Not defined is not identical to undefined
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes- tests and/or benchmarks are included
- documentation is changed or added
- commit message follows commit guidelines
nodejs-github-bot added doc
Issues and PRs related to the documentations.
Issues and PRs related to JavaScript errors originated in Node.js core.
labels
@@ -35,7 +35,7 @@ are handled using the [`try…catch` construct][try-catch] provided by the |
---|
JavaScript language. |
```js |
// Throws with a ReferenceError because z is undefined |
// Throws with a ReferenceError because z is not defined |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine as it's an improvement over what's there, but two totally insignificant suggestions to consider:
- Put a period at the end of the comment as it's a full sentence.
- Maybe instead of "not defined", it should be "undeclared" or even "non-existent"? Those are the terms I see used in the MDN article on ReferenceError, so might as well steal from them. (I looked in the spec too but I don't think the language there would be helpful here, at least not as far as I saw.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- for the period I've added it
- I think
not defined
will be closer to the error thrown which will beReferenceError: z is not defined
.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The period isn't showing up in the GitHub interface. Do you need to push or force-push to your origin maybe?)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that, It's now here.
Seems fast-trackable. If you are a Collaborator and you agree, 👍 here!
Trott added the fast-track
PRs that do not need to wait for 48 hours to land.
label
vsemozhetbyt added the author ready
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
targos pushed a commit to targos/node that referenced this pull request
PR-URL: nodejs#26857 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Vse Mozhet Byt vsemozhetbyt@gmail.com
targos pushed a commit that referenced this pull request
PR-URL: #26857 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Vse Mozhet Byt vsemozhetbyt@gmail.com
BethGriggs pushed a commit that referenced this pull request
PR-URL: #26857 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Vse Mozhet Byt vsemozhetbyt@gmail.com
MylesBorins pushed a commit that referenced this pull request
PR-URL: #26857 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Vse Mozhet Byt vsemozhetbyt@gmail.com
This was referenced
May 29, 2019
This was referenced
May 29, 2019
Labels
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs related to the documentations.
Issues and PRs related to JavaScript errors originated in Node.js core.
PRs that do not need to wait for 48 hours to land.