FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: add more predefined color codes to inspect.colors by BridgeAR · Pull Request #30659 · nodejs/node · GitHub

/ node Public

util: add more predefined color codes to inspect.colors - #30659

Closed
BridgeAR wants to merge 5 commits into
nodejs:masterfrom
BridgeAR:improve-user-options-perf
Closed

util: add more predefined color codes to inspect.colors#30659
BridgeAR wants to merge 5 commits into
nodejs:masterfrom
BridgeAR:improve-user-options-perf

Conversation

Copy link
Copy Markdown
Member
  1. commit: util: improve inspect's customInspect performance
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.
  1. commit: util: add more predefined color codes to inspect.colors
This adds most commonly used ANSI color codes to
`util.inspect.colors`.

@nodejs/util PTAL

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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`.
nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 26, 2019

This comment has been minimized.

Comment thread doc/api/util.md Outdated
Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 26, 2019

Trott commented Nov 26, 2019

Copy link
Copy Markdown
Member

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?

BridgeAR commented Nov 26, 2019
edited
Loading

Copy link
Copy Markdown
Member Author

@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.

BridgeAR added the performance Issues and PRs related to the performance of Node.js. label Nov 26, 2019

Copy link
Copy Markdown
Collaborator

});
}

defineColorAlias('gray', 'grey');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

grey/gray seem motivated, but why are the rest of these here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Collaborator

BridgeAR commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

@nodejs/util this could use another review :)

BridgeAR commented Dec 6, 2019

Copy link
Copy Markdown
Member Author

@nodejs/util @nodejs/repl @devsnek @Trott I would like to land this soon. I have multiple PRs that need more color support.

I'll land this if there's no objection in the next 24 hours.

antsmartian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

This comment has been minimized.

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 6, 2019

nodejs-github-bot commented Dec 6, 2019
edited by BridgeAR
Loading

Copy link
Copy Markdown
Collaborator

BridgeAR added a commit that referenced this pull request Dec 6, 2019
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 <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
BridgeAR added a commit that referenced this pull request Dec 6, 2019
This adds most commonly used ANSI color codes to
`util.inspect.colors`.

PR-URL: #30659
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

BridgeAR commented Dec 6, 2019

Copy link
Copy Markdown
Member Author

Landed in 2a0ec9c, 832290a 🎉

BridgeAR closed this Dec 6, 2019
targos pushed a commit that referenced this pull request Dec 9, 2019
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 <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
targos pushed a commit that referenced this pull request Dec 9, 2019
This adds most commonly used ANSI color codes to
`util.inspect.colors`.

PR-URL: #30659
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
MylesBorins mentioned this pull request Dec 13, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
This adds most commonly used ANSI color codes to
`util.inspect.colors`.

PR-URL: #30659
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
BridgeAR deleted the improve-user-options-perf branch January 20, 2020 12:07
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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 <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This adds most commonly used ANSI color codes to
`util.inspect.colors`.

PR-URL: #30659
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. performance Issues and PRs related to the performance of Node.js. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL