Skip to content

n-api: update cli documentation - #20301

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:n-api-update-cli-docs
Closed

n-api: update cli documentation#20301
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:n-api-update-cli-docs

Conversation

@gabrielschulhof

@gabrielschulhofgabrielschulhof commented Apr 25, 2018

Copy link
Copy Markdown
Contributor

Fixes: #20082

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 cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations. labels Apr 25, 2018
@gabrielschulhofgabrielschulhof added the node-api Issues and PRs related to the Node-API. label Apr 25, 2018
Trott
Trott reviewed Apr 25, 2018
View reviewed changes
Comment threaddoc/api/cli.md Outdated

@TrottTrottApr 25, 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.

Micro-nit:

Allow Node.js to load N-API modules. This option is a no-op. It is kept for
compatibility.

@TrottTrottApr 25, 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.

Or maybe even better, just the last two sentences:

This option is a no-op. It is kept for compatibility.

cjihrig
cjihrig approved these changes Apr 25, 2018
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I'd rather just remove it completely though.

@gabrielschulhof

gabrielschulhof commented Apr 25, 2018

Copy link
Copy Markdown
ContributorAuthor

@Trott I updated the wording as you requested.

@vsemozhetbytvsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 25, 2018
@vsemozhetbyt

vsemozhetbyt commented Apr 25, 2018

Copy link
Copy Markdown
Contributor

Please, add 👍 here to approve fast-tracking.

@vsemozhetbyt

vsemozhetbyt commented Apr 25, 2018

Copy link
Copy Markdown
Contributor

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

@vsemozhetbytvsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2018
@richardlau

richardlau commented Apr 25, 2018

Copy link
Copy Markdown
Member

I'd rather just remove it completely though.

It was previously half removed by #14902 but #19878 put them back.

@gabrielschulhof

gabrielschulhof commented Apr 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Landed in 24c8f0b.

gabrielschulhof pushed a commit that referenced this pull request Apr 26, 2018
Gabriel Schulhof
n-api: update cli documentation
24c8f0b
Fixes: #20082
PR-URL: #20301
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@gabrielschulhof
gabrielschulhof deleted the n-api-update-cli-docs branch April 26, 2018 15:20
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Gabriel Schulhof@MylesBorins
n-api: update cli documentation
8d24b6e
Fixes: #20082
PR-URL: #20301
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request May 8, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott left review comments
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
@richardlaurichardlaurichardlau approved these changes
@trivikrtrivikrtrivikr approved these changes
+1 more reviewer
@vsemozhetbytvsemozhetbytvsemozhetbyt approved these changes
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.cliIssues and PRs related to the Node.js command line interface.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.

documentation of --napi-modules

8 participants

@gabrielschulhof@vsemozhetbyt@richardlau@Trott@addaleax@cjihrig@trivikr@nodejs-github-bot