Skip to content

Conversation

@fhinkel
Copy link
Member

@fhinkelfhinkel commented Dec 2, 2017

Update return type of Init function in documentation to match
napi_addon_register_func signature. Return type used to be
void, now it is napi_value.

Checklist
  • make lint (UNIX) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@fhinkelfhinkel added the node-api Issues and PRs related to the Node-API. label Dec 2, 2017
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Dec 2, 2017
@fhinkelfhinkel requested a review from mhdawsonDecember 2, 2017 11:32
@fhinkelfhinkelforce-pushed the dec/napi-doc-return-value branch from 17cdd4e to 4517c71CompareDecember 2, 2017 13:14
Copy link
Member

Choose a reason for hiding this comment

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

Hm – I think this was intentionally C code? At least that’s what the ```c seems to hint at…

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

But the other examples have ```C and use nullptr. I'm assuming C and c are the same?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe change them all to ```C++? (Yes, it's valid.)

Copy link
Member

@lpincalpincaDec 3, 2017

Choose a reason for hiding this comment

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

I think @addaleax's point is that this is mixing c and c++ code?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@lpinca yes, but the other examples that are labeled as C use nullptr in this file as well, so we're using C++ most of the time anyways. I don't care what we use in the documentation, but it should be consistent throughout the file.

Copy link
Member

Choose a reason for hiding this comment

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

The API must be usable from C, I'm wondering if we should make the examples C code to avoid any confusion over that but don't feel super strongly about that. Either way we should be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

@fhinkel if you want to restrict this PR to the original signature change I'm happy to take on going through to make sure we as consistent throughout the doc.

Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`.
@fhinkelfhinkelforce-pushed the dec/napi-doc-return-value branch from 4517c71 to fd2e875CompareDecember 4, 2017 17:46
@fhinkel
Copy link
MemberAuthor

@mhdawson I've restricted the change to the SayHello example. I'm using nullptr like many other examples in the doc. Please take another look.

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

@fhinkel
Copy link
MemberAuthor

Landed in 2475676

@fhinkelfhinkel closed this Dec 5, 2017
fhinkel added a commit that referenced this pull request Dec 5, 2017
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. PR-URL: #17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. PR-URL: #17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. PR-URL: #17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. PR-URL: #17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@gibfahn
Copy link
Member

@fhinkel FYI the land-on-v8.x means (confusingly) that it's already landed-on-v8.x, and is added when we actually cherry-pick it.

If you want to note that something should go back to v8.x (which is great, please do add the labels), the one you want is lts-watch-v8.x.

@gibfahngibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. PR-URL: #17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
@fhinkel
Copy link
MemberAuthor

@gibfahn Thanks! Sorry for using the wrong label. Thanks for fixing/cherry-picking.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. PR-URL: nodejs#17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Update return type of `Init` function in documentation to match `napi_addon_register_func` signature. Return type used to be `void`, now it is `napi_value`. Backport-PR-URL: #19447 PR-URL: #17424 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@fhinkel@gibfahn@jasnell@Trott@addaleax@lpinca@mhdawson@nodejs-github-bot