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.

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

util: inspect: do not crash on Symbol function names #56572

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 12, 2025

See #56570 - this avoids a crash when a Function name is a Symbol.

@ljharb ljharb added the util Issues and PRs related to the built-in util module. label Jan 12, 2025
@ljharb ljharb requested a review from BridgeAR January 12, 2025 20:55
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 12, 2025
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Jan 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (464f335) to head (cda0416).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56572      +/-   ##
==========================================
- Coverage   89.20%   89.20%   -0.01%     
==========================================
  Files         662      662              
  Lines      191794   191794              
  Branches    36915    36921       +6     
==========================================
- Hits       171084   171083       -1     
+ Misses      13564    13562       -2     
- Partials     7146     7149       +3     
Files with missing lines Coverage Δ
lib/internal/util/inspect.js 99.95% <100.00%> (+0.04%) ⬆️

... and 25 files with indirect coverage changes

@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jan 13, 2025
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2025
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

We should not call inspect() from inside a inspect call. Otherwise the indentation is potentially off.

@ljharb ljharb force-pushed the inspect-symbol-fn-names branch from 6932162 to cda0416 Compare January 13, 2025 16:25
@ljharb ljharb requested a review from BridgeAR January 13, 2025 16:25
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@ljharb ljharb 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 Jan 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@nodejs-github-bot

This comment was marked as outdated.

@ljharb ljharb added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@nodejs-github-bot

This comment was marked as outdated.

@aduh95
Copy link
Contributor

aduh95 commented Jan 13, 2025

@ljharb Please do not add the label when no commits were pushed since previous CI

If there are Jenkins CI failures unrelated to the change in the pull request,
try "Resume Build". It is in the left navigation of the relevant
`node-test-pull-request` job. It will preserve all the green results from the
current job but re-run everything else. Start a fresh CI if more than seven days
have elapsed since the original failing CI as the compiled binaries for the
Windows and ARM platforms are only kept for seven days.
If new commits are pushed to the pull request branch after the latest Jenkins
CI run, a fresh CI run is required. It can be started by pressing "Retry" on
the left sidebar, or by adding the `request-ci` label to the pull request.

@nodejs-github-bot

This comment was marked as outdated.

@ljharb
Copy link
Member Author

ljharb commented Jan 13, 2025

ah ok, thanks

@nodejs-github-bot
Copy link
Collaborator

@ljharb ljharb added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 14, 2025
@aduh95 aduh95 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 15, 2025
@aduh95 aduh95 merged commit 2570f95 into nodejs:main Jan 15, 2025
60 of 69 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Jan 15, 2025

Landed in 2570f95

@ljharb ljharb deleted the inspect-symbol-fn-names branch January 15, 2025 00:38
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 pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants