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

Revert "console: colorize console error and warn" #54677

Merged
merged 1 commit into from
Sep 28, 2024
Merged

Revert "console: colorize console error and warn" #54677

merged 1 commit into from
Sep 28, 2024

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Aug 31, 2024 •
edited
Loading

Fixes #53661

This PR reverts #51629.

Why was this change made?

IMO The original PR introduces more issues than it resolves, negatively impacting the intended use of standard streams in several ways, such as:

Misuse of Standard Streams: The primary purpose of stdout is to handle program output, whereas stderr is designated for diagnostics, status updates, and runtime user messages. Showing this output red may confuse users, as this content isn't always an error. (For example, run ncu-ci with no args...)

Inconsistency in Color Handling: If the input passed to console.error or console.warn already includes color formatting, the additional internal coloring introduced by the original PR can result in a clash of styles.

CC @MrJithil

@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 Aug 31, 2024
@RedYetiDev RedYetiDev mentioned this pull request Aug 31, 2024
Closed
@RedYetiDev RedYetiDev added the revert PRs that revert previously landed PRs. label Aug 31, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.61%. Comparing base (4c844a2) to head (1b725b5).
Report is 615 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54677      +/-   ##
==========================================
+ Coverage   87.33%   87.61%   +0.27%     
==========================================
  Files         650      650              
  Lines      182832   182817      -15     
  Branches    35067    35371     +304     
==========================================
+ Hits       159670   160166     +496     
+ Misses      16421    15927     -494     
+ Partials     6741     6724      -17
Files with missing lines Coverage Δ
lib/internal/console/constructor.js 100.00% <100.00%> (+0.14%) ⬆️

... and 58 files with indirect coverage changes

@Jason3S
Copy link

Jason3S commented Sep 1, 2024

@RedYetiDev,

Thank you!

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 17, 2024 •
edited
Loading

@nodejs/console WDYT

@MrJithil I'd love your review... as it reverts a PR you authored.

@MoLow
Copy link
Member

MoLow commented Sep 20, 2024

I am -1 on reverting but won't block

@RedYetiDev
Copy link
Member Author

I am -1 on reverting but won't block

Why?

joyeecheung
joyeecheung approved these changes Sep 27, 2024
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Personally I find the stderr being marked red somewhat annoying, because stderr is also used quite often for debugging logs and there's nothing erroneous about them, it also misleads you into viewing warnings as errors, and reading a long error stack trace marked red to track things down has been very unpleasant to my eyes. I think it would be better to go with a $LS_COLORS like approach that allows users to use colors that works well with their shell/their use case.

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2d77ba5 into nodejs:main Sep 28, 2024
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2d77ba5

targos pushed a commit that referenced this pull request Oct 4, 2024
@RedYetiDev @targos
Revert "console: colorize console error and warn"
9764002 
This reverts commit a833c9e.

PR-URL: #54677
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
@RedYetiDev @targos
Revert "console: colorize console error and warn"
1dcc5ee 
This reverts commit a833c9e.

PR-URL: #54677
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@RedYetiDev @louwers
Revert "console: colorize console error and warn"
146d4c1 
This reverts commit a833c9e.

PR-URL: nodejs#54677
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
kevinoid added a commit to kevinoid/noderegression that referenced this pull request Nov 2, 2024
@kevinoid
test: handle console colorization
b0f47a0 
The output of console.Console is colorized in Node.js >=20.15 <22.10
due to nodejs/node#51629 and
nodejs/node#54677.  Handle this in our test
cases by comparing the output to the output of console.Console on a
PassThrough stream.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@alexsch01 alexsch01 mentioned this pull request Nov 5, 2024
Open
@RedYetiDev RedYetiDev added lts-watch-v18.x PRs that may need to be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x labels Nov 5, 2024
@alexsch01 alexsch01 mentioned this pull request Nov 5, 2024
Closed
@alexsch01
Copy link
Contributor

@RedYetiDev lts-watch-v18.x can be removed as no commits in v18.x-staging have the bad behavior

@RedYetiDev RedYetiDev removed the lts-watch-v18.x PRs that may need to be released in v18.x. label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@targos targos targos approved these changes

@MrJithil MrJithil Awaiting requested review from MrJithil

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. lts-watch-v20.x PRs that may need to be released in v20.x needs-ci PRs that need a full CI run. revert PRs that revert previously landed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

console.error unexpectedly turns output red
8 participants
@RedYetiDev @Jason3S @MoLow @nodejs-github-bot @alexsch01 @targos @joyeecheung @aduh95