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: de-emphasize wrapping in napi_define_class #36159

Closed

Conversation

gabrielschulhof
Copy link
Contributor

Change the documentation for napi_define_class in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: @gabrielschulhof
Fixes: #36150

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

Change the documentation for `napi_define_class` in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: nodejs#36150
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 18, 2020

Review requested:

  • @nodejs/n-api

@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 Nov 18, 2020
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

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 Show resolved Hide resolved
Copy link
Member

@Trott Trott 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 or without the suggestions I've left

Co-authored-by: Rich Trott <rtrott@gmail.com>
gabrielschulhof pushed a commit that referenced this pull request Nov 20, 2020
Change the documentation for `napi_define_class` in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Fixes: #36150
PR-URL: #36159
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in bb3cbba.

@gabrielschulhof gabrielschulhof deleted the fix-napi-define-class-doc branch November 20, 2020 04:30
codebytere pushed a commit that referenced this pull request Nov 22, 2020
Change the documentation for `napi_define_class` in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Fixes: #36150
PR-URL: #36159
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Change the documentation for `napi_define_class` in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Fixes: #36150
PR-URL: #36159
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Change the documentation for `napi_define_class` in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Fixes: #36150
PR-URL: #36159
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Change the documentation for `napi_define_class` in such a way that
it mentions wrapping C++ class instances as a possible use for the API,
rather than making the assumption that it is the use case for the API.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Fixes: #36150
PR-URL: #36159
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.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.

Modify doc for napi_define_class to be less C++-specific
4 participants