error,test: migrating error to internal/errors #11505
Conversation
nodejs-github-bot
added
assert
|
Minor nit: there's a typo in the commit message: s/intenal/internal/ |
lib/internal/errors.js
Outdated
| @@ -85,4 +85,5 @@ module.exports = exports = { | |||
| // | |||
| // Note: Please try to keep these in alphabetical order | |||
| E('ERR_ASSERTION', (msg) => msg); | |||
| E('ERR_ARG_FUNCTION', '"block" argument must be a function'); | |||
There was a problem hiding this comment.
I think ERR_INVALID_ARG_TYPE would be more suitable for this, refs: https://github.com/nodejs/node/pull/11294/files, I believe the idea at the moment is that concurrent PRs can duplicate some of the error codes introduced in others in order to get a little bit more consistency.
Also whichever the error code would be, can you add the corresponding documentation for it? See Documenting new errors.
test/parallel/test-assert.js
Outdated
| 'TypeError: "block" argument must be a function'); | ||
| assert.strictEqual( | ||
| e.toString(), | ||
| 'TypeError[ERR_ARG_FUNCTION]: "block" argument must be a function'); |
There was a problem hiding this comment.
Can you use common.expectsError for this? Also watch out for #11512
There was a problem hiding this comment.
#11512 has landed, so lines 584-595 can now be something like this:
const validationFunction = common.expectsError({
code: 'ERR_ARG_FUNCTION',
type: TypeError,
message: '"block" argument must be a function'
});
assert.throws(() => { method(block); }, validationFunction);
larissayvette
changed the title
|
Rebased, resolved conflicts, nits addressed, removing Are these things semver-major? PTAL! |
|
Yes, they are semver-major for the initially switch over to internal/errors due to the likelihood of changing error messages and the fact that none of this has been ported back to any release branch. |
Trott
added
the
semver-major
|
|
||
| assert.ok(threw); | ||
| testBlockTypeError(assert.throws, 'string'); |
There was a problem hiding this comment.
Just a suggestion, but I think it would be nice if these could be turned into
testBlockTypeError(() => assert.throws('string')); etc.
There was a problem hiding this comment.
@addaleax Tempted to leave this PR as-is just so I can put your suggestion on a list for Code + Learn... Is that unhealthy? :-P
There was a problem hiding this comment.
@addaleax Having a hard time writing this up for Code + Learn in a way that makes it clear why we want to do this. (Probably because I see it as a style preference.) Would you want to try? If not, no biggie, I'll just leave it off the list. We have enough tasks for the next Code + Learn.
PR-URL: nodejs#11505 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed in 1c834e7 |
PR-URL: nodejs#11505 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Checklist
Affected core subsystem(s)
error,lib
Assign error code to
TypeErrorinassert.js