Conversation
2a4e78e to
7407009
Compare
|
Does this remove the need for #19908? If yes, then it can be closed safely. |
|
@AyushG3112 I kept one of the two checks that are changed in #19908. Your PR is very likely going to land first, so you can just keep that open. |
lib/internal/errors.js
Outdated
There was a problem hiding this comment.
Shouldn't we keep this check? Without this, the error can be implemented without passing in actual too, so no matter what the user passes as the actual argument, the error message will say undefined
There was a problem hiding this comment.
I just moved the check so it will be executed for all errors. That is actually what this PR is all about.
This makes sure the input arguments get validated so implementation errors will be caught early. It also improves a couple of error messages by providing more detailed information and fixes errors detected by the new functionality. Besides that a error type got simplified and tests got refactored.
7407009 to
76a8ce5
Compare
|
Rebased due to conflicts. New CI https://ci.nodejs.org/job/node-test-pull-request/14220/ |
|
|
||
| if (string.length > 0 && (length < 0 || offset < 0)) | ||
| throw new ERR_BUFFER_OUT_OF_BOUNDS('length', true); | ||
| throw new ERR_BUFFER_OUT_OF_BOUNDS(); |
There was a problem hiding this comment.
Can you explain this and the change to fs? I didn't expect to see those here.
There was a problem hiding this comment.
The length was never used in this case. It was a weird API that I fixed.
Originally the implementation was like:
function a (a, b) {
if (b === true) {
return 'foo'
}
return `${a} foo`
}Now it is:
function a(a = undefined) {
if (a === undefined) {
return 'foo'
}
return `${a} foo`
}| E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError); | ||
| E('ERR_UNHANDLED_ERROR', | ||
| (err) => { | ||
| (err = undefined) => { |
There was a problem hiding this comment.
I don't think this is truly needed. Or is it?
There was a problem hiding this comment.
Sadly that is necessary. Default arguments do not count towards function.length as in:
function a(a) {}
function b(a = undefined) {}
assert.strictEqual(a.length, 1);
assert.strictEqual(b.length, 0);Otherwise it is difficult to validate a couple of functions, but I think it is worth it because it makes sure we make less errors while implementing the errors.
There was a problem hiding this comment.
Can you add a comment before the function then? Otherwise it won't be clear to the reader.
| if (isWriting) { | ||
| return 'Attempt to write outside buffer bounds'; | ||
| } else { | ||
| function bufferOutOfBounds(name = undefined) { |
There was a problem hiding this comment.
I don't think the default argument is truly needed.
This makes sure the input arguments get validated so implementation errors will be caught early. It also improves a couple of error messages by providing more detailed information and fixes errors detected by the new functionality. Besides that a error type got simplified and tests got refactored. PR-URL: nodejs#19924 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in dca7fb2 |
This makes sure the input arguments get validated so implementation errors will be caught early. It also improves a couple of error messages by providing more detailed information and fixes errors detected by the new functionality. Besides that a error type got simplified and tests got refactored. PR-URL: #19924 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This makes sure the input arguments get validated so implementation
errors will be caught early. It also improves a couple of error
messages by providing more detailed information and fixes errors
detected by the new functionality. Besides that a error type got
simplified and tests got refactored.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes