errors,tty: migrate to use internal/errors.js #13240
Conversation
nodejs-github-bot
added
errors
lib/internal/errors.js
Outdated
| @@ -148,6 +148,7 @@ E('ERR_SOCKET_BAD_TYPE', | |||
| E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data'); | |||
| E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536'); | |||
| E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running'); | |||
| E('ERR_INVALID_FD', 'fd must be positive integer'); | |||
There was a problem hiding this comment.
These are supposed to be kept alphabetical. (See comment on line 112.) Of course, it doesn't help that the SOCKET ones are all after UNKNOWN and the comment says "Add new errors from here..." so not exactly your fault but if you can fix the ordering while you're in here anyway, that would be great by me.
There was a problem hiding this comment.
Yeah, at this point in time it's a best effort while we get all these done. There are many separate PRs and things are going to get out of order. We need to just stay on top of making sure they get organized eventually.
There was a problem hiding this comment.
To be consistent with the existing error message, something like the following would be appropriate:
E('ERR_INVALID_FD', (fd) => `"fd" must be a positive integer: ${fd}`);
| @@ -38,7 +38,7 @@ function ReadStream(fd, options) { | |||
| if (!(this instanceof ReadStream)) | |||
| return new ReadStream(fd, options); | |||
| if (fd >> 0 !== fd || fd < 0) | |||
| throw new RangeError('fd must be positive integer: ' + fd); | |||
| throw new errors.RangeError('ERR_INVALID_FD', fd); | |||
There was a problem hiding this comment.
You're passing the fd in as an argument but you're not using it in the error message.
There was a problem hiding this comment.
@jasnell fixed as per your suggestion
| @@ -1,14 +1,18 @@ | |||
| // Flags: --expose-internals | |||
There was a problem hiding this comment.
Is the --expose-internals needed here ?
|
The |
|
@mhdawson --expose-internals has been removed, Thanks for the review. |
|
@jasnell Thanks for the review , --expose-internals in the test has been removed. Thanks |
|
@200GAUTAM thanks, CI run: https://ci.nodejs.org/job/node-test-pull-request/8508/ Will plan to land later today. |
|
CI good. Failure was unrelated. Opened #13498 to track the failure. Going to land. |
|
Thanks @200GAUTAM, landed as 3630ed1 |
PR-URL: #13240 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
@mhdawson Thanks for your help |
PR-URL: #13240 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
tty, errors
ref: #11273
This is my first contribution I read and understood the contribution guidelines, please review and suggest.
@jasnell
Thanks.