used es6 conventions for variables and anonymous functions declaration

[ruggertech]

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]

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

[Trott]

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]

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

[ruggertech]

done

[santigimeno]

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

[sam-github]

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

[ruggertech]

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.

[sam-github]

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

[sam-github]

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]

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]

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]

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

[targos]

I think this line can be removed

[gibfahn]

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

[ruggertech]

I do, will take it off

[ruggertech]

do we need anything else for this to be merged ?

[Fishrock123]

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

[ruggertech]

Something like that:

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

?

[ruggertech]

done

[sam-github]

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]

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

[cjihrig]

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

[ruggertech]

Can someone merge please :-)

[targos]

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.

[targos]

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

[jasnell]

Landed in ef74e03.