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

napi: add napi_env to napi_get_last_error_info() #211

Conversation

gabrielschulhof
Copy link
Collaborator

This makes it necessary to pass napi_env to napi_get_last_error_info()
thereby ensuring that errors can be tied to the napi_env in the future.

Re #198

boingoing

This comment was marked as off-topic.

boingoing

This comment was marked as off-topic.

@jasongin
Copy link
Member

should napi_get_last_error_info return a napi_status

I'm leaning toward yes, both for consistency with other APIs and so that it can return an error code if you pass it a null/invalid env. The alternative is to indicate an error via a null return value, but I don't like that as much.

@jasongin
Copy link
Member

You'll need to rebase and merge after my last change. Sorry.

jasongin

This comment was marked as off-topic.

Gabriel Schulhof and others added 2 commits March 29, 2017 15:11
This makes it necessary to pass napi_env to napi_get_last_error_info()
thereby ensuring that errors can be tied to the napi_env in the future.

Re nodejs#198
@jasongin jasongin force-pushed the 198-add-env-to-napi_get_last_error_info branch from 7136ee2 to b6e8694 Compare March 29, 2017 22:41
@jasongin
Copy link
Member

I rebased and pushed a commit to @gabrielschulhof's branch.

@jasongin
Copy link
Member

jasongin commented Mar 29, 2017

boingoing

This comment was marked as off-topic.

@jasongin jasongin merged commit e5461a5 into nodejs:api-prototype-8.x Mar 29, 2017
@gabrielschulhof
Copy link
Collaborator Author

@jasongin @boingoing thanks for jumping in!

@gabrielschulhof gabrielschulhof deleted the 198-add-env-to-napi_get_last_error_info branch March 30, 2017 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants