-
Notifications
You must be signed in to change notification settings - Fork 29.3k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consistent error messages in all modules #892
Conversation
} | ||
|
||
if (msecs < 0 || !isFinite(msecs)) { | ||
throw new RangeError('msecs must be a non-negative finite number'); | ||
throw new RangeError('"msecs" argument must be a positive finite number'); |
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.
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
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.
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. |
+1 on the idea of standardizing error messages. Just a couple I run into:
|
if (Array.isArray(obj)) | ||
throw new TypeError('Arrays are not supported'); | ||
if (n > kMaxLength) | ||
throw new RangeError('n > kMaxLength'); | ||
throw new RangeError('"n" should be lesser than "kMaxLength"'); |
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"
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"?
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!
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 |
I'm in favor of "must". Followed by "Or else." /s |
if (value === undefined) | ||
throw new Error('`value` required in setHeader("' + name + '", value).'); | ||
throw new Error('"value" required in setHeader("' + name + '", value).'); |
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
@@ -307,9 +307,9 @@ function storeHeader(self, state, field, value) { | |||
|
|||
OutgoingMessage.prototype.setHeader = function(name, value) { | |||
if (typeof name !== 'string') | |||
throw new TypeError('`name` should be a string in setHeader(name, value).'); | |||
throw new TypeError('"name" should be a string in setHeader(name, value).'); |
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.
Is this still going places? |
Looks like this is favoured. Can you please rebase and address the comments? |
This commit fixes some error messages that are not consistent with some general rules which most of error messages follow.
@thefourtheye, forgot about this PR, sorry :P |
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., |
Hmmm, this should be targeted at master |
@@ -515,7 +515,7 @@ DiffieHellman.prototype.setPrivateKey = function(key, encoding) { | |||
|
|||
function ECDH(curve) { | |||
if (typeof curve !== 'string') | |||
throw new TypeError('curve should be a string'); | |||
throw new TypeError('"curve" argument should be a string'); |
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?
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
@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. |
@jasnell, @thefourtheye, added #3374 I guess this PR can be closed |
* 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 ```
* 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]>
* 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]>
* 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]>
* 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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
This commit fixes some error messages that are not consistent with
some general rules which most of error messages follow.