Skip to content

src: Reset error struct if error code is napi_ok#40552

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
Closed
JckXia wants to merge 1 commit into from
Closed

src: Reset error struct if error code is napi_ok #40552

JckXia wants to merge 1 commit into from

Conversation

@JckXia
Copy link
Member

@JckXiaJckXia commented Oct 21, 2021

Resetting last_error struct inside the env instance if the error code is napi_ok to make sure that the error code is consistent with any other meta data inside the struct.
ref: nodejs/node-addon-api#1089

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 21, 2021
RaisinTen
RaisinTen approved these changes Oct 23, 2021
View reviewed changes
Copy link
Member

@RaisinTenRaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM. @mhdawson could you take a look at this too?

@JckXiaJckXiaforce-pushed the reset-err-info-if-napi-ok branch from 61c473e to 2bdcfd5CompareOctober 24, 2021 17:15
mhdawson
mhdawson approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actionsgithub-actionsbot mentioned this pull request Oct 26, 2021
Open
25 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 26, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 26, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 26, 2021

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

This was referenced Oct 27, 2021
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 29, 2021

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

This was referenced Oct 30, 2021
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 3, 2021

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

This was referenced Nov 4, 2021
Open
Open
Open
Open
Open
Open
@JckXiaJckXiaforce-pushed the reset-err-info-if-napi-ok branch from 2bdcfd5 to ad97f6eCompareNovember 14, 2021 22:40
@JckXia
Copy link
MemberAuthor

JckXia commented Nov 15, 2021

@mhdawson It looks like the CI has passed after a rebase.

@mhdawsonmhdawson added the node-api Issues and PRs related to the Node-API. label Nov 15, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 15, 2021

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

@mhdawson
Copy link
Member

mhdawson commented Nov 15, 2021

@JckXia I needed to kick off a CI, the passing checks are the ones done through actions.

A commend with the new CI should be added to the issue soon and lets keep our fingers crossed that it passes this time.

@github-actionsgithub-actionsbot mentioned this pull request Nov 16, 2021
Open
40 tasks
@JckXia
Copy link
MemberAuthor

JckXia commented Nov 16, 2021

@mhdawson Hmm it looks like the CI failed again. Will do more digging.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 16, 2021

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

@github-actionsgithub-actionsbot mentioned this pull request Nov 17, 2021
Open
39 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 17, 2021

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

@mhdawson
Copy link
Member

mhdawson commented Nov 17, 2021

@JckXia I think it's still CI flakyness based on the failures, not your PR. I've resumed once more and hopefully we'll get to green.

@JckXia
Copy link
MemberAuthor

JckXia commented Nov 17, 2021

Thank you @mhdawson!

@mhdawson
Copy link
Member

mhdawson commented Nov 17, 2021

It's greeeeen :)

@mhdawson
Copy link
Member

mhdawson commented Nov 17, 2021

Landed in 86e976f

mhdawson pushed a commit that referenced this pull request Nov 17, 2021
@JckXia@mhdawson
src: reset error struct if error code is napi_ok
86e976f
PR-URL: #40552 Refs: nodejs/node-addon-api#1089 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Nov 17, 2021

@JckXia thanks for your patience and work on this one:)

@JckXia
Copy link
MemberAuthor

JckXia commented Nov 17, 2021

@mhdawson Thank you so much! :)

This was referenced Nov 18, 2021
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 21, 2021
@JckXia@targos
src: reset error struct if error code is napi_ok
c2259c9
PR-URL: #40552 Refs: nodejs/node-addon-api#1089 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@targostargos mentioned this pull request Nov 26, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@JckXia@danielleadams
src: reset error struct if error code is napi_ok
6532063
PR-URL: #40552 Refs: nodejs/node-addon-api#1089 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@JckXia@danielleadams
src: reset error struct if error code is napi_ok
7237bcc
PR-URL: #40552 Refs: nodejs/node-addon-api#1089 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
Merged
@JckXiaJckXia deleted the reset-err-info-if-napi-ok branch February 11, 2022 18:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@gabrielschulhofgabrielschulhofgabrielschulhof approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

@juanarboljuanarboljuanarbol approved these changes

@RaisinTenRaisinTenRaisinTen approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JckXia@nodejs-github-bot@mhdawson@jasnell@gabrielschulhof@juanarbol@RaisinTen