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

First time contribution from NodeConf EU #16821

Closed
wants to merge 3 commits into from

Conversation

paulashfield
Copy link

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 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
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Thanks @paulashfield — just some minor feedback below.

assert.strictEqual(addon.testNapiRun('(41.92 + 0.08);'), 42,
'napi_run_script() works correctly');
assert.throws(() => addon.testNapiRun({ abc: 'def' }), /string was expected/);
const args = [41.92, 0.08];
Copy link
Member

Choose a reason for hiding this comment

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

This would be a bit nicer as just, say:

const val1 = 41.92;
const val2 = 0.08;

const args = [41.92, 0.08];
const testCase = '(' + args.join(' + ') + ');';
const expected = 43;
const actual = addon.testNapiRun(testCase);
Copy link
Member

Choose a reason for hiding this comment

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

This could be then rewritten as:

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

assert.throws(() => addon.testNapiRun({ abc: 'def' }), /string was expected/);
const args = [41.92, 0.08];
const testCase = '(' + args.join(' + ') + ');';
const expected = 43;
Copy link
Member

Choose a reason for hiding this comment

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

This could then be const expected = val1 + val2;

const expected = 43;
const actual = addon.testNapiRun(testCase);

assert.strictEqual(actual, expected, `testNapiRun${testCase} gets ${actual} not expected ${expected}`);
Copy link
Member

Choose a reason for hiding this comment

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

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

@apapirovski 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
Copy link
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
Copy link
Contributor

const actual = addon.testNapiRun(testCase);

assert.strictEqual(actual, expected, `testNapiRun${testCase} gets ${actual} not expected ${expected}`);
assert.throws(() => addon.testNapiRun({abc: 'def'}), /string was expected/);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@paulashfield
Copy link
Author

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

@Trott
Copy link
Member

Trott commented Nov 7, 2017

@Trott
Copy link
Member

Trott commented Nov 8, 2017

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?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Needs spacing flagged by linter fixed...

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Nov 8, 2017

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2017
PR-URL: nodejs#16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@Trott
Copy link
Member

Trott commented Nov 8, 2017

Landed in 3ee524b.
Thanks for the contribution! 🎉

@Trott Trott closed this Nov 8, 2017
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
PR-URL: #16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@gibfahn
Copy link
Member

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 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

image

gibfahn pushed a commit that referenced this pull request Dec 13, 2017
PR-URL: #16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This was referenced Dec 20, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
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
Backport-PR-URL: #19447
PR-URL: #16821
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants