GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Consistent error messages in all modules by micnic · Pull Request #892 · nodejs/node

Conversation

@micnic

@micnic micnic commented Feb 19, 2015

Copy link
Copy Markdown
Contributor

This commit fixes some error messages that are not consistent with
some general rules which most of error messages follow.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig reviewed Feb 19, 2015
View reviewed changes
Comment thread lib/timers.js Outdated

@cjihrig cjihrig Feb 19, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"non-negative" and "positive" are not quite the same thing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@micnic micnic Feb 19, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I used "positive" to fit in 80 characters line length, but I can switch it back

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@silverwind silverwind Mar 19, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Probably best to split the string then.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cjihrig

cjihrig commented Feb 19, 2015

Copy link
Copy Markdown
Contributor

I'm +1 on standardizing error messages. My only real question is, have we actually defined a standard for this? Based on this PR, it looks like it would be to capitalize at the beginning of the message, and to put argument names in double quotes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@piscisaureus

piscisaureus commented Feb 19, 2015

Copy link
Copy Markdown
Contributor

+1 on the idea of standardizing error messages.
Maybe it's good to discuss a template for them first?

Just a couple I run into:

Error: connect ENOENT does.not.exist
Error: getaddrinfo ENOTFOUND does.not.exist.com does.not.exist.com:81
Error: ENOENT: no such file or directory, open 'C:\Users\Bert Belder\bar\baz'
RangeError: port should be > 0 and < 65536: 0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
silverwind
silverwind reviewed Mar 19, 2015
View reviewed changes
Comment thread lib/smalloc.js Outdated

@silverwind silverwind Mar 19, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this should be "n" should be less or equal than "kMaxLength"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kenany kenany Mar 19, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"less than or equal to"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@silverwind silverwind Mar 19, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That's even better!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@timoxley

timoxley commented Mar 19, 2015

Copy link
Copy Markdown
Contributor

Perhaps this is for another PR, but perhaps should make the usage of "should" vs "must" consistent – currently they seem to be used interchangeably:

e.g.

throw new TypeError('"options" argument must be an object');

vs

throw new TypeError('"curve" argument should be a string');

I'd suggest that once the code is throwing an assertion error it's out of the jurisdiction of a "should" and into the hands of "must", though perhaps "must" sounds a little pushy so perhaps io.js could optimise for developer feels and give people an illusion of choice with the calm & assertive "should".

Some related literature http://www.differencebetween.net/language/grammar-language/difference-between-should-and-must/

edit: I suggest following these common definitions used by RFCs in "Key word for use in RFCs to Indicate Requirement Levels": https://tools.ietf.org/html/rfc2119
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@micnic micnic mentioned this pull request Mar 20, 2015
Consistent error messages in all modules #1220
Closed
@bnoordhuis

bnoordhuis commented Mar 20, 2015

Copy link
Copy Markdown
Member

I'm in favor of "must". Followed by "Or else."

/s
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brendanashworth
brendanashworth reviewed Mar 24, 2015
View reviewed changes
Comment thread lib/_http_outgoing.js Outdated

@brendanashworth brendanashworth Mar 24, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

required -> is required

I'd also say that we shouldn't replace "name" with the actual value of the variable

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@chrisdickinson chrisdickinson added the land-on-master label Apr 17, 2015
ronkorving
ronkorving reviewed Jul 11, 2015
View reviewed changes
Comment thread lib/_http_outgoing.js Outdated

@ronkorving ronkorving Jul 11, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nowhere (except in this file) do your error messages end with a period.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ronkorving

ronkorving commented Aug 10, 2015

Copy link
Copy Markdown
Contributor

Is this still going places?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Aug 10, 2015

Copy link
Copy Markdown
Contributor

Looks like this is favoured. Can you please rebase and address the comments?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@micnic
Consistent error messages in all modules
f28b73a 
This commit fixes some error messages that are not consistent with
some general rules which most of error messages follow.
@micnic

micnic commented Aug 10, 2015

Copy link
Copy Markdown
Contributor Author

@thefourtheye, forgot about this PR, sorry :P
rebased and took in count all the comments added
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Aug 10, 2015

Copy link
Copy Markdown
Member

LGTM if it passes CI. 👍

Maybe @nodejs/documentation can find an appropriate place for a minimal Error message standardization doc based on the existing error messages (if such a doc doesn't already exist). Just to get it off the ground, the doc might only contains the two things @cjihrig notes (double quotes, capitalize first word unless there's a compelling reason not to such as it is a case-sensitive variable name) along with a note about omitting final punctuation.

I would prefer a standard that had punctuation at the end and made verbs explicit (e.g., Index is out of range. rather than Index out of range) but consistency is way more important than my own somewhat arbitrary preferences.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Aug 10, 2015

Copy link
Copy Markdown
Contributor

Hmmm, this should be targeted at master
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
r-52
r-52 reviewed Aug 10, 2015
View reviewed changes
Comment thread lib/crypto.js

@r-52 r-52 Aug 10, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Small question: Every other error is something line ".... must be ...". Does it make sense to change it also in crypto?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@micnic micnic Aug 10, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We don't have a compromise on this yet (should vs must) see https://tools.ietf.org/html/rfc2119

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Oct 14, 2015

Copy link
Copy Markdown
Member

@micnic ... in general this is a good step but it definitely needs to be targeted at master and rebased. It won't be able to land as is.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@micnic micnic mentioned this pull request Oct 14, 2015
Consistent error messages in all modules #3374
Closed
@micnic

micnic commented Oct 14, 2015

Copy link
Copy Markdown
Contributor Author

@jasnell, @thefourtheye, added #3374 I guess this PR can be closed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@micnic micnic closed this Oct 14, 2015
Trott added a commit to Trott/io.js that referenced this pull request Feb 8, 2017
@Trott
test: refactor test-fs-buffer
efe334c 
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: 2a is hex value for * and not catch-stdout-error.js
```
Trott added a commit to Trott/io.js that referenced this pull request Feb 10, 2017
@Trott
test: refactor test-fs-buffer
05be623 
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: nodejs#11232
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit that referenced this pull request Feb 13, 2017
@Trott Italo A. Casas
test: refactor test-fs-buffer
f8f4b33 
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: #11232
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@Trott Italo A. Casas
test: refactor test-fs-buffer
ccd1163 
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: nodejs#11232
Reviewed-By: James M Snell <[email protected]>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
@Trott @krydos
test: refactor test-fs-buffer
f54f884 
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'nodejsGH-1899-output.js',
  'nodejsGH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: nodejs#11232
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
@elvisphilipn @BridgeAR
test: corret assertion arg order in test-regress-nodejsGH-892.js
f4323d8 
PR-URL: nodejs#23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
@elvisphilipn @BridgeAR
test: corret assertion arg order in test-regress-nodejsGH-892.js
183838d 
PR-URL: nodejs#23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@elvisphilipn @jasnell
test: corret assertion arg order in test-regress-GH-892.js
e4e8983 
PR-URL: #23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@elvisphilipn @addaleax
test: corret assertion arg order in test-regress-GH-892.js
b9c99cf 
PR-URL: #23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@elvisphilipn @MylesBorins
test: corret assertion arg order in test-regress-GH-892.js
a098528 
PR-URL: #23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
v10.13.1 proposal #24675
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@elvisphilipn @rvagg
test: corret assertion arg order in test-regress-GH-892.js
f42d5da 
PR-URL: #23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@elvisphilipn @MylesBorins
test: corret assertion arg order in test-regress-GH-892.js
a059180 
PR-URL: #23513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
v10.14.2 proposal #24727
Merged
@snyk-bot snyk-bot mentioned this pull request Jan 31, 2022
[Snyk] Security upgrade shelljs from 0.7.8 to 0.8.5 enterstudio/node#26
Open
@lpinca lpinca mentioned this pull request May 19, 2022
deps: upgrade to libuv 1.44.2 #42340
Closed
@aliscco aliscco mentioned this pull request Apr 27, 2023
[Snyk] Fix for 30 vulnerabilities aliscco/alisco-node#279
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

14 participants

@micnic @cjihrig @piscisaureus @timoxley @bnoordhuis @ronkorving @thefourtheye @Trott @jasnell @silverwind @kenany @r-52 @brendanashworth @chrisdickinson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.