errors: multiple updates #15002
errors: multiple updates #15002
Conversation
nodejs-github-bot
added
the
errors
| message, | ||
| actual: false, | ||
| expected: true, | ||
| operator: '==' |
There was a problem hiding this comment.
Actually, since this is loose equality, shouldn't it stay as ==? The current assert() function uses == as well (https://github.com/nodejs/node/blob/master/lib/assert.js#L78)
There was a problem hiding this comment.
@evanlucas if I'm not wrong there only assert.strictEqual() is currently used in this file.
Edit: nvm didn't see https://github.com/nodejs/node/pull/15002/files#diff-5a3344c263a73c663dd1cfcb91880fd7R70. Yes it's better to keep ==, ignore my comment.
There was a problem hiding this comment.
IMHO since this is an internal module assertions should be made in /test/, so there's no need for runtime assertions at all.
|
@nodejs/collaborators ... ping! |
|
As am I, to be honest, I'm not convinced that it will be all that useful. |
| message, | ||
| actual: false, | ||
| expected: true, | ||
| operator: '==' |
There was a problem hiding this comment.
IMHO since this is an internal module assertions should be made in /test/, so there's no need for runtime assertions at all.
|
@refack... that's certainly a valid argument also :-) |
|
I agree with @refack that using AssertionErrors here is not required and I would actually like to get rid of all asserts in I am also not convinced about the export of the constants. |
|
Looking at it further, I don't think we should remove the asserts. In particular, if someone ends up passing something like I will drop the commit that exports the constants tho. @refack ... let's look at the possibility of pulling the asserts separately. |
jasnell
force-pushed
the
improve-errors-internals
branch
from
7f85568
to
6208225
Compare
|
Well, I did not mean to get rid of the checks but I think it is good enough if we use Another note - as I pointed out in #14350, this is not really a alternative to it as it tries to solve something different. |
IMHO assertions are valid when dealing with userland inputs. For an internal (and fairly simple) construct, coverage by unit tests should be sufficient. But anyway I'm removing my objection. |
|
@jasnell do you mind changing the assert name to something else e.g. "internalAssert"? Otherwise I think it is a bit confusing that it is not the "real" assert module and no other functions are available. |
I just had the same though 🤔 |
jasnell
force-pushed
the
improve-errors-internals
branch
from
ec87cad
to
2e894b3
Compare
|
Updated with the function renamed and a test fixed. |
PR-URL: #15002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
|
Landed in 6ff521b |
PR-URL: nodejs#15002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
|
This will need to be backported for 8.x |
PR-URL: nodejs#15002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15002 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
An alternative to #14350 ... eliminates the circular dependency with the
assertmodule.Also, export the defined error keys as constants. Some have said this makes it easier but I'm not fully convinced.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
errors