-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add more predefined color codes to inspect.colors #30659
Conversation
This improves the performance to copy user options that are then passed through to the custom inspect function. The performance improvement depends on the complexity of the custom inspect function. For very basic cases this is 100% faster than before.
This adds most commonly used ANSI color codes to `util.inspect.colors`.
This comment has been minimized.
This comment has been minimized.
I'd consider adding more colors that we support to be semver-minor. Once we add it, we'll risk breaking people's stuff if we ever take it away. Not strongly opposed (at least at this moment), but wondering if we might want to leave robust color support to userland modules and just support the relatively small number of colors we already support. Is there a justification for these colors in particular over the colors that we already have? |
@Trott I actually already have follow-up pull requests that add further "robust" color support to Node.js. This is just a first step in that direction. I recently had another PR open (#29676) where I needed a color code that we did not yet have in our predefined ones. We also need more APIs to properly handle colors internally. Otherwise it's not possible to add colors in Node.js core and that is something I try to slowly move forward. |
This comment has been minimized.
This comment has been minimized.
}); | ||
} | ||
|
||
defineColorAlias('gray', 'grey'); |
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.
grey/gray seem motivated, but why are the rest of these 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.
Mainly due to different descriptions depending on where these ANSI codes are described. I have no strong opinion on either of these names. I thought it does not hurt to allow different aliases so people could use what ever they feel most comfortable with. The camel case and all lower cased versions are just there to prevent typos.
@nodejs/util this could use another review :) |
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.
LGTM
This comment has been minimized.
This comment has been minimized.
This improves the performance to copy user options that are then passed through to the custom inspect function. The performance improvement depends on the complexity of the custom inspect function. For very basic cases this is 100% faster than before. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This adds most commonly used ANSI color codes to `util.inspect.colors`. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This improves the performance to copy user options that are then passed through to the custom inspect function. The performance improvement depends on the complexity of the custom inspect function. For very basic cases this is 100% faster than before. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This adds most commonly used ANSI color codes to `util.inspect.colors`. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This improves the performance to copy user options that are then passed through to the custom inspect function. The performance improvement depends on the complexity of the custom inspect function. For very basic cases this is 100% faster than before. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This adds most commonly used ANSI color codes to `util.inspect.colors`. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This improves the performance to copy user options that are then passed through to the custom inspect function. The performance improvement depends on the complexity of the custom inspect function. For very basic cases this is 100% faster than before. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
This adds most commonly used ANSI color codes to `util.inspect.colors`. PR-URL: #30659 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anto Aravinth <[email protected]>
util: improve inspect's customInspect performance
util: add more predefined color codes to inspect.colors
@nodejs/util PTAL
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes