used es6 conventions for variables and anonymous functions declaration #9669

ruggertech · 2016-11-17T22:59:25Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Description of change

changed es5 "var" to "const" and anonymous functions to arrow function syntax

mscdex · 2016-11-17T23:02:54Z

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

Trott · 2016-11-18T00:20:22Z

If we're going to refactor the test for mostly stylistic reasons, I'd ask that we also change the three instances of assert.equal() to assert.strictEqual().

cjihrig · 2016-11-18T03:22:50Z

I agree with @Trott. I think the process.nextTick() that surrounds those assertions should also have a common.mustCall().

ruggertech · 2016-11-18T08:21:52Z

done

santigimeno · 2016-11-18T11:51:58Z

test/parallel/test-fs-read-stream-err.js

-    assert.equal(err_, err);
+  process.nextTick(() => {
+    assert.strictEqual(stream.fd, null);
+    assert.strictEqual(err_, err);


Use common.mustCall() to make sure those assertions a run, as @cjihrig suggested

sam-github · 2016-11-18T14:25:28Z

test/parallel/test-fs-read-stream-err.js

-  process.nextTick(function() {
-    assert.equal(stream.fd, null);
-    assert.equal(err_, err);
+  process.nextTick(() => {


why did you use an () => here, and leave function() on line 11?

because I saw that in line 11 the previous developer used a named function expression, contrary to other places where he used an anonymous function in the old form.

Named function expressions can be handy when errors are thrown and I believe that was the reason for using a named function on line 11.
I didn't want to break this behavior.

I see your thinking, but think its too subtle, function names don't matter, particularly not in tests. Better change them all.

sam-github · 2016-11-18T14:27:55Z

const arguably does something useful, es6 functions don't (unless you are trying to access the enclosing this). I don't really mind all the code churn, but if you are going to change some functions to es6, please do them all, so the file is internally consistent.

ruggertech · 2016-11-18T22:49:10Z

I changed all the functions in the code except for the function declaration on line 11 which was originally set as a named function, for the sake of easier debugging, by the original developer. I changed all of the anonymous functions to their respective es6 arrow function form, but did not want to affect code stability or ease of debugging by changing line 11.

Should line 11 also be changed?

sam-github · 2016-11-18T22:51:55Z

I think none should be changed, but if we are going to go all es6y, lets go all the way, not half way.

I'm a bit worried that the wave of "name functions for readability of stack traces" is going to crash against the wave of "use the new es6 function syntax", and leave confusion, but for now, all es6 in this test is fine.

ruggertech · 2016-11-18T23:22:11Z

I see your point. Removed the named function to maintain consistency across this file.

targos · 2016-11-19T16:22:12Z

test/parallel/test-fs-read-stream-err.js


-stream.on('error', common.mustCall(function errorHandler(err_) {
+stream.on('error', common.mustCall((err_) => {
  console.error('error event');


I think this line can be removed

@ruggertech Do you agree with @targos 's suggestion here?

I do, will take it off

ruggertech · 2016-11-20T22:43:28Z

do we need anything else for this to be merged ?

Fishrock123 · 2016-11-20T22:45:27Z

test/parallel/test-fs-read-stream-err.js

 };

-stream.on('data', function(buf) {
+stream.on('data', (buf) => {
  stream.on('data', common.fail);  // no more 'data' events should follow


It would be better not not invoke common.fail directly but rather wrap it in a function and pass it a message string.

Something like that:

stream.on('data', () => {
stream.on('data', () => common.fail('no more "data" events should follow'));
});

?

sam-github · 2016-11-21T16:57:58Z

commit message doesn't follow guidelines, it's too long and doesn't start with test: , see https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

bjouhier · 2016-11-21T17:20:16Z

test/parallel/test-fs-read-stream-err.js

-stream.on('data', function(buf) {
-  stream.on('data', common.fail);  // no more 'data' events should follow
+stream.on('data', () => {
+  stream.on('data', () => common.fail);  // no more 'data' events should follow


should be either () => common.fail() or just common.fail.

cjihrig · 2016-11-27T13:41:20Z

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

ruggertech · 2016-11-30T08:53:51Z

Can someone merge please :-)

targos · 2016-12-03T12:04:50Z

test/parallel/test-fs-read-stream-err.js

-    var cb = arguments[arguments.length - 1];
-    process.nextTick(function() {
+  fs.read = common.mustCall(() => {
+    const cb = arguments[arguments.length - 1];


arguments now refers to the outer function's arguments. It doesn't change the outcome of the test but I think it's better not to change this one to an arrow function.

implemented arrow functions and changed var to const where appropriate.

targos · 2016-12-03T18:52:48Z

Thanks. LGTM. Final CI: https://ci.nodejs.org/job/node-test-pull-request/5155/

implemented arrow functions and changed var to const where appropriate. PR-URL: #9669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-12-05T23:20:39Z

Landed in ef74e03.

implemented arrow functions and changed var to const where appropriate. PR-URL: #9669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

implemented arrow functions and changed var to const where appropriate. PR-URL: nodejs#9669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

implemented arrow functions and changed var to const where appropriate. PR-URL: #9669 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 17, 2016

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 17, 2016

santigimeno reviewed Nov 18, 2016

View reviewed changes

sam-github reviewed Nov 18, 2016

View reviewed changes

targos approved these changes Nov 19, 2016

View reviewed changes

cjihrig approved these changes Nov 20, 2016

View reviewed changes

Fishrock123 reviewed Nov 20, 2016

View reviewed changes

bjouhier reviewed Nov 21, 2016

View reviewed changes

targos reviewed Dec 3, 2016

View reviewed changes

ruggertech closed this Dec 3, 2016

test: implemented es6 conventions

b209404

implemented arrow functions and changed var to const where appropriate.

ruggertech reopened this Dec 3, 2016

jasnell approved these changes Dec 5, 2016

View reviewed changes

jasnell closed this Dec 5, 2016

addaleax added the land-on-v6.x label Dec 8, 2016

MylesBorins added the land-on-v4.x label Dec 20, 2016

This was referenced Dec 21, 2016

V6.9.3 proposal #10394

Merged

V4.7.1 proposal #10395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

used es6 conventions for variables and anonymous functions declaration #9669

used es6 conventions for variables and anonymous functions declaration #9669

ruggertech commented Nov 17, 2016 •

edited

Loading

mscdex commented Nov 17, 2016

Trott commented Nov 18, 2016

cjihrig commented Nov 18, 2016

ruggertech commented Nov 18, 2016

santigimeno Nov 18, 2016

sam-github Nov 18, 2016

ruggertech Nov 18, 2016

sam-github Nov 18, 2016

sam-github commented Nov 18, 2016

ruggertech commented Nov 18, 2016

sam-github commented Nov 18, 2016

ruggertech commented Nov 18, 2016

targos Nov 19, 2016

gibfahn Nov 22, 2016

ruggertech Nov 24, 2016 •

edited

Loading

ruggertech commented Nov 20, 2016

Fishrock123 Nov 20, 2016 •

edited

Loading

ruggertech Nov 20, 2016 •

edited

Loading

ruggertech Nov 21, 2016

sam-github commented Nov 21, 2016

bjouhier Nov 21, 2016

cjihrig commented Nov 27, 2016

ruggertech commented Nov 30, 2016

targos Dec 3, 2016

targos commented Dec 3, 2016

jasnell commented Dec 5, 2016

used es6 conventions for variables and anonymous functions declaration #9669

used es6 conventions for variables and anonymous functions declaration #9669

Conversation

ruggertech commented Nov 17, 2016 • edited Loading

Checklist

Description of change

mscdex commented Nov 17, 2016

Trott commented Nov 18, 2016

cjihrig commented Nov 18, 2016

ruggertech commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Nov 18, 2016

ruggertech commented Nov 18, 2016

sam-github commented Nov 18, 2016

ruggertech commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruggertech Nov 24, 2016 • edited Loading

Choose a reason for hiding this comment

ruggertech commented Nov 20, 2016

Fishrock123 Nov 20, 2016 • edited Loading

Choose a reason for hiding this comment

ruggertech Nov 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Nov 21, 2016

Choose a reason for hiding this comment

cjihrig commented Nov 27, 2016

ruggertech commented Nov 30, 2016

Choose a reason for hiding this comment

targos commented Dec 3, 2016

jasnell commented Dec 5, 2016

ruggertech commented Nov 17, 2016 •

edited

Loading

ruggertech Nov 24, 2016 •

edited

Loading

Fishrock123 Nov 20, 2016 •

edited

Loading

ruggertech Nov 20, 2016 •

edited

Loading