-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util: refactor util module #13803
util: refactor util module #13803
Conversation
lib/util.js
Outdated
const errors = require('internal/errors'); | ||
|
||
const isError = internalUtil.isError; | ||
const { errname } = process.binding('uv'); | ||
const { isBuffer } = require('buffer').Buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the .Buffer be removed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, isBuffer
is a property of Buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow yea. Disregard.
@@ -564,12 +586,12 @@ function formatValue(ctx, value, recurseTimes) { | |||
braces[0] = `${constructor.name} ${braces[0]}`; | |||
|
|||
if (empty === true) { | |||
return braces[0] + base + braces[1]; | |||
return `${braces[0]}${base}${braces[1]}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it improves readability in this specific case, but LGTM either way.
* refactor util exports * early capture of prototype methods * use template strings and args consistently
Rebased... updated. |
* refactor util exports * early capture of prototype methods * use template strings and args consistently PR-URL: #13803 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Landed in b1355ba |
This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR. |
Unless it ends up causing backport pains for other PRs, this likely isn't a priority to backport. |
Okay, I’ve switched to dont-land. I’m still a bit worried with Node 8 ending LTS in almost 2 years, and we already have a lot of backporting pain… |
Actually.. I'm going to do a backport for this, it does interfere with backporting another one. Sigh. |
* refactor util exports * early capture of prototype methods * use template strings and args consistently PR-URL: nodejs#13803 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
* refactor util exports * early capture of prototype methods * use template strings and args consistently Backport-PR-URL: #14585 Backport-Reviewed-By: Anna Henningsen <[email protected]> PR-URL: #13803 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@jasnell LTS? |
Not unless it's blocking anything. |
Refactor util module
exports =
also so monkeypatching still works)...args
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
util