Application #1
Open
Application #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About the application:
In order to make the review of the application as easy as possible I created 3 different commits:
Test that Object::GetPropertyNames does not return properties whose key is a Symbol
As detailed in the documentation when a
Symbolis used as key, it should not be returned byObject::GetPropertyNames.Test that Object::Get returns undefined if the key does not exist
The docs point out that
Object::Getmust returnundefinedif the key does not exist. I added an extra test for that case and made a quick refactor to avoid repetition.Add test for Object::InstanceOf
The
Object::InstanceOfmethod was missing some tests. I noticed that the rule is to create separate files only if the method to test is overloaded but, for the sake of demonstrating that I understand how to add new files, I created the tests in separate files.I also double checked that the API was available since N-API v1, so no
#ifdefrequired.About me:
Hi! My name is Jose and I'd love to join the Node.js N-API Working Group as a mentee.
At the moment I work as a full stack developer (React + Node.js) and it'll be great to give something back to the FLOSS project that pays my bills 😄
Even though I started my professional career as a C++ developer, nowadays I use C++ for open source projects. Mainly Linux related stuff.
You can have a look to my most popular side project (C++) if you want.
I also collaborate with other organizations trying to make the Linux desktop more user friendly. Here are some cool GIFs 😉