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

doc: add clarification for exception behaviour #25339

Closed
wants to merge 6 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Jan 4, 2019

Refs: nodejs/abi-stable-node#356

Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Refs: nodejs/abi-stable-node#356

Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Jan 4, 2019
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated
called in order to allow for some minimal cleanup to be completed
before returning to JavaScript. For those functions, `napi_status`
will reflect the success/error/exception for that function, irrespective of
whether an exception was pending when the function was called.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is difficult to understand. "for that function" is particularly confusing coming after "for those functions". Maybe try writing it as two shorter sentences or something? Or just tidying up the sentence that's there somehow?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this?

In that case, `napi_status` will reflect the status for the function.
It will not reflect previous pending exceptions.

Copy link
Member

Choose a reason for hiding this comment

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

(Obviously, change my suggestion if it is factually incorrect. I'm commenting on style and readability, not content.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Your suggestion works for me. Updating.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with @Trott’s suggestions

Trott and others added 5 commits January 7, 2019 11:19
Co-Authored-By: mhdawson <michael_dawson@ca.ibm.com>
Co-Authored-By: mhdawson <michael_dawson@ca.ibm.com>
Co-Authored-By: mhdawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member Author

mhdawson commented Jan 7, 2019

@Trott fixed up as per your comments.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 8, 2019

@danbev
Copy link
Contributor

danbev commented Jan 9, 2019

Landed in b406c9c.

@danbev danbev closed this Jan 9, 2019
danbev pushed a commit that referenced this pull request Jan 9, 2019
Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

PR-URL: #25339
Refs: nodejs/abi-stable-node#356
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

PR-URL: #25339
Refs: nodejs/abi-stable-node#356
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

PR-URL: nodejs#25339
Refs: nodejs/abi-stable-node#356
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

PR-URL: #25339
Refs: nodejs/abi-stable-node#356
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
BethGriggs pushed a commit that referenced this pull request May 10, 2019
Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

PR-URL: #25339
Refs: nodejs/abi-stable-node#356
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Document current behaviour where some methods can be called
when an exception is pending, while others cannot and explain
the behaviour.

PR-URL: #25339
Refs: nodejs/abi-stable-node#356
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. 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