First time contribution from NodeConf EU#16821
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added dont-land-on-v4.x node-api 
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
Thanks @paulashfield — just some minor feedback below.
There was a problem hiding this comment.
This would be a bit nicer as just, say:
constval1=41.92;constval2=0.08;There was a problem hiding this comment.
This could be then rewritten as:
constactual=addon.testNapiRun(`(${val1} + ${val2});`);There was a problem hiding this comment.
This could then be const expected = val1 + val2;
There was a problem hiding this comment.
Here you could remove the third argument (the error message) as the default message communicates the same thing.
apapirovski added the code-and-learn 
gireeshpunathil commented Nov 6, 2017
@paulashfield - 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
@paulashfield See also the last note in this chapter: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#setting-up-your-local-environment |
There was a problem hiding this comment.
A nit: the last line seems lacking the last line break.
paulashfield commented Nov 7, 2017
Many thanks all for feedback - really helped. I have amended and recommitted, hope that looks better. |
Trott commented Nov 7, 2017
Trott commented Nov 8, 2017
Hi @paulashfield! Welcome and thanks for the PR! If you run |
Trott left a comment
Trott left a comment There was a problem hiding this comment.
Needs spacing flagged by linter fixed...
Trott left a comment
Trott left a comment There was a problem hiding this comment.
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
PR-URL: nodejs#16821 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Trott commented Nov 8, 2017
Landed in 3ee524b. |
PR-URL: #16821 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
gibfahn commented Dec 13, 2017
@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 That's why there's a
|
PR-URL: #16821 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs#16821 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Backport-PR-URL: #19447 PR-URL: #16821 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)