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

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

Closed
wants to merge 1 commit into from

Conversation

JckXia
Copy link
Member

@JckXia JckXia 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-bot nodejs-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
Copy link
Contributor

@RaisinTen RaisinTen 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?

Copy link
Member

@mhdawson mhdawson 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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@JckXia
Copy link
Member Author

JckXia commented Nov 15, 2021

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

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

@mhdawson
Copy link
Member

@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.

@JckXia
Copy link
Member Author

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
Copy link
Collaborator

@mhdawson
Copy link
Member

@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
Member Author

JckXia commented Nov 17, 2021

Thank you @mhdawson!

@mhdawson
Copy link
Member

It's greeeeen :)

@mhdawson
Copy link
Member

Landed in 86e976f

@mhdawson mhdawson closed this Nov 17, 2021
mhdawson pushed a commit that referenced this pull request Nov 17, 2021
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@mhdawson
Copy link
Member

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

@JckXia
Copy link
Member Author

JckXia commented Nov 17, 2021

@mhdawson Thank you so much! :)

targos pushed a commit that referenced this pull request Nov 21, 2021
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
@JckXia JckXia deleted the reset-err-info-if-napi-ok branch February 11, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants