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

util: update ansi regex #54865

Closed
wants to merge 2 commits into from
Closed

util: update ansi regex #54865

wants to merge 2 commits into from
+38 −7

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Sep 9, 2024 •
edited
Loading

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 9, 2024
@RedYetiDev RedYetiDev marked this pull request as draft September 9, 2024 15:50
@RedYetiDev RedYetiDev marked this pull request as ready for review September 9, 2024 15:52
@BridgeAR BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Sep 9, 2024
@marco-ippolito
Copy link
Member

I would prefer to have a test

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2024

@marco-ippolito it is tested in the library. We could of course copy those tests but I believe in this case, that's not necessary, WDYT?

@codecov Codecov
Copy link

codecov bot commented Sep 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.90%. Comparing base (c3a7b29) to head (3c24916).
Report is 158 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54865      +/-   ##
==========================================
- Coverage   87.91%   87.90%   -0.01%     
==========================================
  Files         651      651              
  Lines      183354   183357       +3     
  Branches    35719    35717       -2     
==========================================
- Hits       161190   161181       -9     
- Misses      15446    15459      +13     
+ Partials     6718     6717       -1
Files with missing lines Coverage Δ
lib/internal/util/inspect.js 99.95% <100.00%> (+<0.01%) ⬆️

... and 35 files with indirect coverage changes

@RedYetiDev RedYetiDev added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Sep 9, 2024
@RedYetiDev
Copy link
Member Author

I've split this into two commits. One for fixing the regex, and the other for adding the tests.

RedYetiDev
RedYetiDev commented Sep 9, 2024
View reviewed changes
lib/internal/util/inspect.js
Comment on lines +244 to +245
// Ref: https://github.com/chalk/ansi-regex/blob/f338e1814144efb950276aac84135ff86b72dc8e/index.js
// License: MIT by Sindre Sorhus <sindresorhus@gmail.com>
Copy link
Member Author

@RedYetiDev RedYetiDev Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Review note: I've changed this from an authors list, as the number of authors has grown since this was last updated.

CC @sindresorhus to make sure this is appropiate

Copy link

Choose a reason for hiding this comment

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

Yes

@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 9, 2024

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/10782645643

@RedYetiDev RedYetiDev added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

CI LGTM

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev RedYetiDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 18, 2024
@RedYetiDev
Copy link
Member Author

Can this land?

@marco-ippolito
Copy link
Member

Can this land?

CI is red

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 20, 2024 •
edited
Loading

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

Oh okay, can you resume?

@nodejs-github-bot
Copy link
Collaborator

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

jasnell pushed a commit that referenced this pull request Sep 21, 2024
@RedYetiDev @jasnell
util: update ansi regex
e42ad5e 
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 21, 2024
@RedYetiDev @jasnell
test: add util.stripVTControlCharacters test
9416354 
PR-URL: #54865
Refs: chalk/ansi-regex#58
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 21, 2024

Landed in 5b3f3c5...9416354

@jasnell jasnell closed this Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus left review comments

@jasnell jasnell jasnell approved these changes

@MoLow MoLow MoLow approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@marco-ippolito marco-ippolito marco-ippolito 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. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

util.stripVTControlCharacters does not strip hyperlinks
7 participants
@RedYetiDev @marco-ippolito @BridgeAR @nodejs-github-bot @jasnell @sindresorhus @MoLow