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

test: change to arrow functions in send-bad-arguments #23483

Conversation

annatangzhao
Copy link
Contributor

@annatangzhao annatangzhao commented Oct 12, 2018

Part of Code and Learn workshop at Node + JS Interactive!

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 12, 2018
@targos targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@@ -28,17 +28,17 @@ const buf = Buffer.from('test');
const host = '127.0.0.1';
const sock = dgram.createSocket('udp4');

assert.throws(function() {
Copy link
Member

Choose a reason for hiding this comment

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

Since the function contains only one statement, the curly brackets are not required here and other places in the code.
That means, () => { sock.send(); } can be () => sock.send()

Copy link
Member

@Trott Trott Oct 14, 2018

Choose a reason for hiding this comment

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

In my opinion, the brackets should not be removed in this situation. They should only be removed if the arrow function is explicitly returning a value (in my opinion).

Copy link
Member

@Trott Trott Oct 14, 2018

Choose a reason for hiding this comment

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

In other words:

() => { return foo(); } should be changed to () => foo(); but () => { foo(); } should be left as-is, in my opinion. Otherwise, someone reading the code is left wondering if the fact that the arrow function is returning a value is a result of the author wanting to omit the brackets for the sake of omitting the brackets or if it's because the function is meant to return a value.

@Trott Trott mentioned this pull request Oct 14, 2018
@Trott
Copy link
Member

Trott commented Oct 14, 2018

@Trott
Copy link
Member

Trott commented Oct 15, 2018

Landed in 6783eed

@Trott Trott closed this Oct 15, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 15, 2018
PR-URL: nodejs#23483
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23483
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
PR-URL: #23483
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
PR-URL: #23483
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23483
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23483
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants