FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

First time contribution from NodeConf EU by paulashfield · Pull Request #16821 · nodejs/node · GitHub

/ node Public

First time contribution from NodeConf EU - #16821

Closed
paulashfield wants to merge 3 commits into
nodejs:masterfrom
paulashfield:master
ConversationCommits (3)ChecksFiles changed
Closed

First time contribution from NodeConf EU#16821
paulashfield wants to merge 3 commits into
nodejs:masterfrom
paulashfield:master

Conversation

paulashfield commented Nov 6, 2017

Copy link
Copy Markdown
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Nov 6, 2017
apapirovski reviewed Nov 6, 2017
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @paulashfield — just some minor feedback below.

Comment thread test/addons-napi/test_general/testNapiRun.js Outdated

apapirovski Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This would be a bit nicer as just, say:

const val1 = 41.92;
const val2 = 0.08;

Comment thread test/addons-napi/test_general/testNapiRun.js Outdated

apapirovski Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This could be then rewritten as:

const actual = addon.testNapiRun(`(${val1} + ${val2});`);

Comment thread test/addons-napi/test_general/testNapiRun.js Outdated

apapirovski Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This could then be const expected = val1 + val2;

Comment thread test/addons-napi/test_general/testNapiRun.js Outdated

apapirovski Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Here you could remove the third argument (the error message) as the default message communicates the same thing.

apapirovski added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017

gireeshpunathil commented Nov 6, 2017

Copy link
Copy Markdown
Member

@paulashfield -
looks like you pushed changed without:

#git config --global user.name
#git config --global user.email

on your system - implication of which is that this commit will not be associated with your profile. Can you set them up and push once again?

vsemozhetbyt commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

@paulashfield See also the last note in this chapter: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#setting-up-your-local-environment

vsemozhetbyt reviewed Nov 6, 2017
View reviewed changes
Comment thread test/addons-napi/test_general/testNapiRun.js Outdated

vsemozhetbyt Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A nit: the last line seems lacking the last line break.

paulashfield commented Nov 7, 2017

Copy link
Copy Markdown
Author

Many thanks all for feedback - really helped. I have amended and recommitted, hope that looks better.

Trott commented Nov 7, 2017

Copy link
Copy Markdown
Member

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

Trott commented Nov 8, 2017

Copy link
Copy Markdown
Member

Hi @paulashfield! Welcome and thanks for the PR! If you run make lint-js (or vcbuild lint-js if you're on Windows), you will see a report of a linting error on line 15. Could you fix that up?

Trott requested changes Nov 8, 2017
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Needs spacing flagged by linter fixed...

Trott approved these changes Nov 8, 2017
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because the lint issue was just about adding two spaces, I went ahead and did it myself. Hope that's OK.

Trott commented Nov 8, 2017

Copy link
Copy Markdown
Member

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

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2017
test: improve error emssage reporting in testNapiRun.js
3ee524b 
PR-URL: nodejs#16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Trott commented Nov 8, 2017

Copy link
Copy Markdown
Member

Landed in 3ee524b.
Thanks for the contribution! 🎉

Trott closed this Nov 8, 2017
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
test: improve error emssage reporting in testNapiRun.js
5d3a4ad 
PR-URL: #16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
evanlucas mentioned this pull request Nov 13, 2017
Merged
LJNeon mentioned this pull request Nov 15, 2017
Closed

gibfahn commented Dec 13, 2017

Copy link
Copy Markdown
Member

@paulashfield just an FYI, this commit isn't associated with your Github account. You need to go to https://github.com/settings/emails and add paul.ashfield@intercity.technology. Then all your commits will be associated with you.

That's why there's a ? next to your name on the commit: 3ee524b

gibfahn pushed a commit that referenced this pull request Dec 13, 2017
test: improve error emssage reporting in testNapiRun.js
25ff7bf 
PR-URL: #16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This was referenced Dec 20, 2017
Closed
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
test: improve error emssage reporting in testNapiRun.js
ab95aa4 
PR-URL: nodejs#16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
test: improve error emssage reporting in testNapiRun.js
2476ab9 
Backport-PR-URL: #19447
PR-URL: #16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins mentioned this pull request Apr 16, 2018
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
gireeshpunathil gireeshpunathil approved these changes
+2 more reviewers
apapirovski apapirovski left review comments
vsemozhetbyt vsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL