Conversation
lib/console.js
Outdated
There was a problem hiding this comment.
I'd say just go ahead and write out WRITABLE_STREAM instead of abbreviating
lib/internal/errors.js
Outdated
There was a problem hiding this comment.
Please use the new common.expectsError() method here.
|
Thank you for feedback. |
|
Yes, please add it to the docs using the pattern seen in the other PRs #11273 |
|
This looks like a duplicate of #11308, I've added that one to the tracking issue because that one is earlier. |
|
@joyeecheung yes unfortunately I didn't see it before I started. |
|
@mskec Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Could you rebase and also squash your commits (I think all the changes should be one commit, right?). Thanks! |
|
@fhinkel I rebased and squashed commits. Let me know if there is anything else |
| E('ERR_ARG_NOT_ITERABLE', '%s must be iterable'); | ||
| E('ERR_ASSERTION', (msg) => msg); | ||
| E('ERR_CONSOLE_WRITABLE_STREAM', | ||
| (name) => `Console expects a writable stream instance for ${name}`); |
There was a problem hiding this comment.
Why not use the %s syntax already used by several other format strings?
There was a problem hiding this comment.
No special reason. I made this PR when there was no other error messages using %s syntax.
Will change if you think that way is better
|
Thanks. Landed in 0ecdf29. |
Migrate console.js to use internal/errors.js.
Refs: #11273
cc @jasnell
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)