Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tty: add hasColors function #26247

Closed
wants to merge 1 commit into from
Closed

tty: add hasColors function #26247

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

This adds a small wrapper around the getColorDepth function to check
if the stream supports at least a specific amount of colors. This is
convenient as the other API is not as straight forward and most use
cases likely only want to know if a specific amount of colors is
supported or not.

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

@BridgeAR BridgeAR requested a review from addaleax February 21, 2019 20:08
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2694/pipeline

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Feb 21, 2019
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 21, 2019
addaleax
addaleax approved these changes Feb 21, 2019
View reviewed changes
doc/api/tty.md Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/20952/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2019
@BridgeAR
Copy link
Member Author

Resumed CI https://ci.nodejs.org/job/node-test-commit/26074/

@silverwind silverwind mentioned this pull request Feb 22, 2019
Closed
3 tasks
vsemozhetbyt
vsemozhetbyt reviewed Feb 22, 2019
View reviewed changes
doc/api/tty.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Feb 26, 2019
View reviewed changes
doc/api/tty.md Outdated
* `count` {integer} The number of colors that are requested (minimum 2).
**Default:** 16.
* `env` {Object} An object containing the environment variables to check.
**Default:** `process.env`.
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit... it's not clear from the documentation how the env bit works here. Perhaps an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two examples below but I'll try to improve the wording further.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 28, 2019 •
edited
Loading

CI https://ci.nodejs.org/job/node-test-pull-request/21047/
https://ci.nodejs.org/job/node-test-pull-request/21053/

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 3, 2019

@jasnell PTAL

@BridgeAR
tty: add hasColors function
42af1e0 
This adds a small wrapper around the `getColorDepth` function to check
if the stream supports at least a specific amount of colors. This is
convenient as the other API is not as straight forward and most use
cases likely only want to know if a specific amount of colors is
supported or not.
@BridgeAR BridgeAR force-pushed the add-tty-has-colors branch from 1c209fb to 42af1e0 Compare March 5, 2019 00:29
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 5, 2019

I am not sure why but Github reported conflicts, so I rebased once more. Locally there was no conflict and I just squashed the commits, no other change.

CI https://ci.nodejs.org/job/node-test-pull-request/21210/

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 5, 2019
@BridgeAR
tty: add hasColors function
d4fdec6 
This adds a small wrapper around the `getColorDepth` function to check
if the stream supports at least a specific amount of colors. This is
convenient as the other API is not as straight forward and most use
cases likely only want to know if a specific amount of colors is
supported or not.

PR-URL: nodejs#26247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 5, 2019

Landed in d4fdec6 🎉

@BridgeAR BridgeAR closed this Mar 5, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@BridgeAR @targos
tty: add hasColors function
070faf0 
This adds a small wrapper around the `getColorDepth` function to check
if the stream supports at least a specific amount of colors. This is
convenient as the other API is not as straight forward and most use
cases likely only want to know if a specific amount of colors is
supported or not.

PR-URL: nodejs#26247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos added a commit that referenced this pull request Mar 27, 2019
@targos
2019-03-28, Version 11.13.0 (Current)
04903d8 
Notable changes:

* events:
  * Added a `once` function to use `EventEmitter` with promises
    (#26078).
* tty:
  * Added a `hasColors` method to `WriteStream`
    (#26247).
  * Added NO_COLOR and FORCE_COLOR support
    (#26485).
* v8:
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools
    (#26501).
* meta:
  * Gireesh Punathil is now a member of the Technical Steering Committee
    (#26657).
  * Added ZYSzys to collaborators (#26730).

PR-URL: #26949
@targos targos mentioned this pull request Mar 27, 2019
Merged
targos added a commit that referenced this pull request Mar 28, 2019
@targos
2019-03-28, Version 11.13.0 (Current)
29867f3 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
targos added a commit that referenced this pull request Mar 28, 2019
@targos
2019-03-28, Version 11.13.0 (Current)
2c73868 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
@targos @BethGriggs
2019-03-28, Version 11.13.0 (Current)
473b162 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
@BridgeAR @BethGriggs
tty: add hasColors function
4285b57 
This adds a small wrapper around the `getColorDepth` function to check
if the stream supports at least a specific amount of colors. This is
convenient as the other API is not as straight forward and most use
cases likely only want to know if a specific amount of colors is
supported or not.

PR-URL: #26247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
@Qix- Qix- mentioned this pull request Jul 12, 2019
Closed
@BridgeAR BridgeAR deleted the add-tty-has-colors branch January 20, 2020 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@starkwang starkwang starkwang approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @nodejs-github-bot @jasnell @thefourtheye @addaleax @vsemozhetbyt @starkwang