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.

Sign up for GitHub

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

Jump to bottom

n-api: update cli documentation #20301

Closed
wants to merge 1 commit into from
Closed

n-api: update cli documentation #20301

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
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-bot nodejs-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
@gabrielschulhof gabrielschulhof 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
doc/api/cli.md Outdated
Enable loading native modules compiled with the ABI-stable Node.js API (N-API)
(experimental).
Allow Node.js to load N-API modules (this option is a no-op - it is kept for
compatibility).
Copy link
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.

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

@cjihrig cjihrig left a comment

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
Copy link
Contributor Author

@Trott I updated the wording as you requested.

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

Please, add 👍 here to approve fast-tracking.

@vsemozhetbyt
Copy link
Contributor

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

@vsemozhetbyt vsemozhetbyt 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
Copy link
Member

I'd rather just remove it completely though.

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

@gabrielschulhof
Copy link
Contributor Author

Landed in 24c8f0b.

gabrielschulhof pushed a commit that referenced this pull request Apr 26, 2018
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
@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>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. node-api Issues 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