Skip to content

improved documentation and fix on include_dir. #4

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Sep 3, 2021

Conversation

NickNaso
Copy link
Member

@NickNaso NickNaso commented Aug 5, 2021

Fixes #3
Added more documentation about node-api-headers
Fixed the include_dir property.

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

LGTM!

@KevinEady
Copy link
Contributor

Would using path.resolve be better actually? 🤷

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

Saw this after approving

- fixed typos
- used path.resolve instead of path.relative.
@NickNaso
Copy link
Member Author

Saw this after approving

@KevinEady I made the requested changes. Please take a look when you have time.

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 once the outstanding comments from @KevinEady and @gabrielschulhof are addressed.

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

LGTM!

@mhdawson mhdawson merged commit d68505e into nodejs:main Sep 3, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion about the next steps
4 participants