Skip to content

n-api: fix napi_property_descriptor documentation - #20433

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:20421-fix-napi-property-descriptor
Closed

n-api: fix napi_property_descriptor documentation#20433
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:20421-fix-napi-property-descriptor

Conversation

@gabrielschulhof

@gabrielschulhofgabrielschulhof commented Apr 30, 2018

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

@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 Apr 30, 2018
@vsemozhetbyt

vsemozhetbyt commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

FWIW, the last fix is also done here: #20430

@gabrielschulhof

gabrielschulhof commented Apr 30, 2018

Copy link
Copy Markdown
ContributorAuthor

@vsemozhetbyt whoops - I'll remove that part.

@gabrielschulhof
gabrielschulhofforce-pushed the 20421-fix-napi-property-descriptor branch from 393be56 to 9af5c4fCompareApril 30, 2018 17:58
@gabrielschulhof

gabrielschulhof commented Apr 30, 2018

Copy link
Copy Markdown
ContributorAuthor

@vsemozhetbyt fixed.

@vsemozhetbyt

vsemozhetbyt commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt

vsemozhetbyt commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

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

@vsemozhetbytvsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 30, 2018
@gabrielschulhofgabrielschulhof mentioned this pull request Apr 30, 2018
Closed
mhdawson
mhdawson reviewed May 3, 2018
View reviewed changes
Comment threaddoc/api/n-api.md Outdated

@mhdawsonmhdawsonMay 3, 2018

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.

I thought the suggestion was to use NULL instead of 0, just wondering why you went the other way?

@gabrielschulhofgabrielschulhofMay 4, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oh, right. I thought it would be more consistent, but you're right. This is plain C. I should actually use NULL everywhere – though that might expand the scope of the PR.

I'll add NULLs everywhere this PR touches, and do another sweep to make sure that the entire doc says NULL.

@gabrielschulhof
gabrielschulhofforce-pushed the 20421-fix-napi-property-descriptor branch from 9af5c4f to 72575f2CompareMay 4, 2018 15:22
@gabrielschulhof

gabrielschulhof commented May 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@mhdawson I have updated the affected parts to show NULL wherever it's needed.

@BridgeAR

BridgeAR commented May 5, 2018

Copy link
Copy Markdown
Member

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/670/

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018
@gabrielschulhof

gabrielschulhof commented May 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Landed in 4692cf4.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request May 6, 2018
Gabriel Schulhof
doc: fix N-API property descriptor documentation
4692cf4
PR-URL: nodejs#20433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gabrielschulhof
gabrielschulhof deleted the 20421-fix-napi-property-descriptor branch May 6, 2018 16:26
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Gabriel Schulhof@MylesBorins
doc: fix N-API property descriptor documentation
0f75c08
PR-URL: #20433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Gabriel Schulhof@MylesBorins
doc: fix N-API property descriptor documentation
e94deb8
PR-URL: #20433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
Gabriel Schulhof@MylesBorins
doc: fix N-API property descriptor documentation
880772a
PR-URL: #20433
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@mhdawsonmhdawsonmhdawson left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.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.

6 participants

@gabrielschulhof@vsemozhetbyt@BridgeAR@cjihrig@mhdawson@nodejs-github-bot