Skip to content

src: improve error handling in process_wrap - #56977

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jasnell:jasnell/process_wrap_errorhandling
Feb 11, 2025
Merged

src: improve error handling in process_wrap#56977
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jasnell:jasnell/process_wrap_errorhandling

Conversation

@jasnell

@jasnelljasnell commented Feb 9, 2025

Copy link
Copy Markdown
Member

Replace ToLocalChecked uses.

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Feb 9, 2025
@nodejs-github-bot

nodejs-github-bot commented Feb 9, 2025•
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

@codecov

codecovBot commented Feb 9, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 41.25000% with 47 lines in your changes missing coverage. Please review.

Project coverage is 89.10%. Comparing base (b181535) to head (d1e10c1).
Report is 28 commits behind head on main.

Files with missing linesPatch %Lines
src/process_wrap.cc41.25%24 Missing and 23 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #56977 +/- ##
==========================================
- Coverage 89.13% 89.10% -0.04% 
==========================================
Files 665 665 Lines 193165 193218 +53 Branches 37191 37208 +17 ==========================================
- Hits 172181 172169 -12 - Misses 13729 13754 +25 - Partials 7255 7295 +40
Files with missing linesCoverage Δ
src/process_wrap.cc67.37% <41.25%> (-16.96%)⬇️

... and 27 files with indirect coverage changes

anonrig
anonrig approved these changes Feb 9, 2025
View reviewed changes
Comment threadsrc/process_wrap.cc
stdios->Get(context, i).ToLocalChecked().As<Object>();
Local<Value> type =
stdio->Get(context, env->type_string()).ToLocalChecked();
Local<Value> val;

@anonriganonrigFeb 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we define this variable outside of the loop? Same goes for 143 and 144.

@jasnelljasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 9, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 888e5eb into nodejs:mainFeb 11, 2025
@nodejs-github-bot

nodejs-github-bot commented Feb 11, 2025

Copy link
Copy Markdown
Collaborator

Landed in 888e5eb

targos pushed a commit that referenced this pull request Feb 17, 2025
@jasnell@targos
src: improve error handling in process_wrap
d0ee8c0
Replace ToLocalChecked uses.
PR-URL: #56977
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@jasnell@acidiney
src: improve error handling in process_wrap
2c5ac73
Replace ToLocalChecked uses.
PR-URL: nodejs#56977
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@github-actionsgithub-actionsBot mentioned this pull request Feb 25, 2025
Merged
aduh95 pushed a commit that referenced this pull request Apr 2, 2025
@jasnell@aduh95
src: improve error handling in process_wrap
1ffd59b
Replace ToLocalChecked uses.
PR-URL: #56977
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
aduh95 pushed a commit that referenced this pull request Apr 3, 2025
@jasnell@aduh95
src: improve error handling in process_wrap
05de504
Replace ToLocalChecked uses.
PR-URL: #56977
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95aduh95 mentioned this pull request Apr 11, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@jasnell@RafaelGSS
src: improve error handling in process_wrap
36b17b7
Replace ToLocalChecked uses.
PR-URL: #56977
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@jasnell@RafaelGSS
src: improve error handling in process_wrap
93aa439
Replace ToLocalChecked uses.
PR-URL: #56977
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig approved these changes
@legendecaslegendecaslegendecas approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.child_processIssues and PRs related to the child_process subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell@nodejs-github-bot@anonrig@legendecas