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

console: colorize console error and warn #51629

Closed
wants to merge 10 commits into from
Closed

console: colorize console error and warn #51629

wants to merge 10 commits into from

Conversation

MrJithil
Copy link
Contributor

@MrJithil MrJithil commented Feb 1, 2024 •
edited

Related to : 40361

Colorise string type args of console.error and console.warn. We are skipping the non-string types.

Tasks:

  • Run CITGM

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. needs-ci PRs that need a full CI run. labels Feb 1, 2024
@MrJithil
Copy link
Contributor Author

MrJithil commented Feb 1, 2024

Will add test cases if this feature is acceptable

@MrJithil MrJithil force-pushed the colorize-console-error-and-warning branch 5 times, most recently from 827fe0c to b69e24d Compare February 1, 2024 13:22
@MrJithil MrJithil force-pushed the colorize-console-error-and-warning branch 8 times, most recently from 0a37526 to 3b4f498 Compare February 5, 2024 11:30
@MrJithil
Copy link
Contributor Author

MrJithil commented Feb 5, 2024

@MoLow I have changed the value of clear from '\u001bc' to '\u001b[0m'. Please check.

@MrJithil MrJithil force-pushed the colorize-console-error-and-warning branch from 3b4f498 to d5ac03b Compare February 5, 2024 11:39
@ruyadorno ruyadorno added the needs-citgm PRs that need a CITGM CI run. label Feb 5, 2024
@ruyadorno
Copy link
Member

ruyadorno commented Feb 5, 2024 •
edited

I believe it's important to run through citgm to gauge the impact of this change.

@MrJithil MrJithil force-pushed the colorize-console-error-and-warning branch from d5ac03b to 3bf4dbf Compare February 6, 2024 11:26
@MrJithil MrJithil force-pushed the colorize-console-error-and-warning branch from 3bf4dbf to d07b7b4 Compare March 14, 2024 09:32
@marco-ippolito
Copy link
Member

I once again believe this should be marked as semver major change: #51503

@marco-ippolito marco-ippolito mentioned this pull request Mar 15, 2024
Open
@marco-ippolito marco-ippolito added the blocked PRs that are blocked by other issues or PRs. label Mar 15, 2024
@aduh95

This comment was marked as outdated.

Sign in to view
@MrJithil MrJithil force-pushed the colorize-console-error-and-warning branch 2 times, most recently from 471a7d4 to 8747c98 Compare March 20, 2024 10:09
@MrJithil MrJithil requested a review from MoLow March 20, 2024 10:10
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@MrJithil MrJithil added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
ruyadorno
ruyadorno approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

I thought a bit more about this after talking with a few other folks and changed my mind on the remaining objection. LGTM 👍

@ruyadorno
Copy link
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3422

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@MrJithil
Copy link
Contributor Author

I thought a bit more about this after talking with a few other folks and changed my mind on the remaining objection. LGTM 👍

thank you. proceeding further.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@MrJithil
Copy link
Contributor Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3422

Could you please help me to run this CIGTM? I haven't run this before and felt a little confusing. Since these tests are time consuming, thought to take help or opinion before retrying it.

@MrJithil MrJithil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 30, 2024
@aduh95
Copy link
Contributor

aduh95 commented May 5, 2024

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3422

Could you please help me to run this CIGTM? I haven't run this before and felt a little confusing. Since these tests are time consuming, thought to take help or opinion before retrying it.

$ ncu-ci.js citgm 3421 3422

FAILURE: 6 failures in 3422 not present in 3421


┌────────────────────────┬──────────────────┬───────────────────────┐
│        (index)         │        0         │           1           │
├────────────────────────┼──────────────────┼───────────────────────┤
│       osx11-x64        │                  │                       │
│      rhel8-s390x       │                  │                       │
│       win-vs2022       │                  │                       │
│ alpine-last-latest-x64 │                  │                       │
│       rhel8-x64        │ 'undici-v6.14.1' │                       │
│      debian12-x64      │                  │                       │
│      debian11-x64      │  'jest-v0.0.0'   │    'semver-v7.6.0'    │
│   fedora-latest-x64    │                  │                       │
│     rhel8-ppc64le      │  'pino-v8.19.0'  │ 'prom-client-v15.1.2' │
│      aix72-ppc64       │                  │                       │
│     ubuntu2204-64      │ 'undici-v6.14.1' │                       │
│   alpine-latest-x64    │                  │                       │
│ fedora-last-latest-x64 │                  │                       │
└────────────────────────┴──────────────────┴───────────────────────┘

Now it's a matter of investigating whether those failures were caused by this PR or something else. For semver and prom-client, that's easy, No space left of device, so unrelated. For the other ones, it would require more careful review – but it seems fair to say landing this PR would not cause to much trouble to the ecosystem.

MrJithil added a commit that referenced this pull request May 8, 2024
@MrJithil
console: colorize console error and warn
a833c9e 
prints console error in red and warn in yellow

PR-URL: #51629
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ruy Adorno <ruy@vlt.sh>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MrJithil
Copy link
Contributor Author

MrJithil commented May 8, 2024

Landed in a833c9e

@MrJithil MrJithil closed this May 8, 2024
@MrJithil MrJithil deleted the colorize-console-error-and-warning branch May 8, 2024 13:59
targos pushed a commit that referenced this pull request May 11, 2024
@MrJithil @targos
console: colorize console error and warn
db76c58 
prints console error in red and warn in yellow

PR-URL: #51629
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ruy Adorno <ruy@vlt.sh>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno removed the blocked PRs that are blocked by other issues or PRs. label May 13, 2024
@targos targos mentioned this pull request May 13, 2024
Merged
@Julian24Design
Copy link

Updated node to latest version, but console.error still outputs unclolored text. Is there any configuration required to achieve this?

lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
@MrJithil @lukins-cz
console: colorize console error and warn
5334376 
prints console error in red and warn in yellow

PR-URL: nodejs#51629
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ruy Adorno <ruy@vlt.sh>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruyadorno ruyadorno ruyadorno approved these changes

@jasnell jasnell jasnell approved these changes

@MoLow MoLow MoLow approved these changes

@aduh95 aduh95 Awaiting requested review from aduh95

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@MrJithil @ruyadorno @marco-ippolito @aduh95 @nodejs-github-bot @Julian24Design @jasnell @MoLow