errors,net: change internal/net.js to use internal/errors.js #11302
Conversation
nodejs-github-bot
added
errors
seppevs
force-pushed
the
internal_net_use_internal_error
branch
from
6f56724
to
3cd2f1e
Compare
lib/internal/errors.js
Outdated
| @@ -86,3 +86,4 @@ module.exports = exports = { | |||
| // Note: Please try to keep these in alphabetical order | |||
| E('ERR_ASSERTION', (msg) => msg); | |||
| // Add new errors from here... | |||
| E('ERR_INVALID_PORT', 'Port should be >= 0 and < 65536'); | |||
There was a problem hiding this comment.
Nit: why not keep the the "port" argument part?
There was a problem hiding this comment.
@joyeecheung : Fixed
seppevs
force-pushed
the
internal_net_use_internal_error
branch
from
3cd2f1e
to
28c2ee2
Compare
lib/internal/errors.js
Outdated
| @@ -86,3 +86,6 @@ module.exports = exports = { | |||
| // Note: Please try to keep these in alphabetical order | |||
| E('ERR_ASSERTION', (msg) => msg); | |||
| // Add new errors from here... | |||
| E('ERR_INVALID_PORT', (port) => { | |||
| return `Port should be >= 0 and < 65536, got "${port}"`; | |||
There was a problem hiding this comment.
While we're changing this, we should make this say must instead of should
There was a problem hiding this comment.
Done: I've changed should to must
seppevs
force-pushed
the
internal_net_use_internal_error
branch
from
28c2ee2
to
d2c3e7e
Compare
| @@ -86,3 +86,6 @@ module.exports = exports = { | |||
| // Note: Please try to keep these in alphabetical order | |||
| E('ERR_ASSERTION', (msg) => msg); | |||
| // Add new errors from here... | |||
| E('ERR_INVALID_PORT', (port) => { | |||
| return `Port must be >= 0 and < 65536, got "${port}"`; | |||
There was a problem hiding this comment.
nit: the block is unnecessary, we can just
(port) => `"port" argument must be >= 0 and < 65536, got ${port}`
EDIT: no need for the quotes around ${port}, that looks like a string but usually it's meant to be a number.
There was a problem hiding this comment.
I'd prefer the quotes in the case it is not a number.
There was a problem hiding this comment.
So should I keep the quotes?
There was a problem hiding this comment.
I am fine with keeping the quotes.
| @@ -575,6 +586,7 @@ found [here][online]. | |||
| [domains]: domain.html | |||
| [event emitter-based]: events.html#events_class_eventemitter | |||
| [file descriptors]: https://en.wikipedia.org/wiki/File_descriptor | |||
| [Node.js Error Codes]: #nodejs-error-codes | |||
There was a problem hiding this comment.
I think it will be #nodejs_error_codes
There was a problem hiding this comment.
There was a problem hiding this comment.
@jasnell Yes, I realized it after a while. Shouldn't we let the tools take care of deciding the ids?
There was a problem hiding this comment.
I've done it this way to ensure that the ID's remain constant even if the automatic link generation changes or the structure of the document is refactored. I much prefer the explicit headers.
There was a problem hiding this comment.
Hmmm, I am not sure. We lose consistency here.
joyeecheung
added
the
semver-major
|
We're finally able to move forward on this, but it's going to need a rebase |
|
I don't think this PR is still necessary: I see no Error messages in |
|
It's possible that things were shifted around since then. Errors still need to be migrated but this particular PR may be too out of date to simply rebase. |
|
@seppevs, thanks for your PR. Sorry that it got dragged out for so long due to being a semver-major change. I'll go ahead and close this. Ping me if it should stay open. |
Change internal/net.js so it makes use of the new internal/errors.js module.
See #11273 for more info.
cc @jasnell
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
errors,net