test: replace function with arrow function by Leko · Pull Request #17345 · nodejs/node · GitHub

This is part of Nodefest's Code and Learn nodejs/code-and-learn#72

Among the list of Code and Learn, I solved the unfinished task of replacing function with arrow function

test/parallel/test-assert.js
test/parallel/test-domain-top-level-error-handler-clears-stack.js
test/parallel/test-querystring.js
test/parallel/test-whatwg-url-searchparams-getall.js
test/parallel/test-writeint.js
test/parallel/test-zerolengthbufferbug.js

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Among the list of [Code and Learn](nodejs/code-and-learn#72 (comment)), I solved the unfinished task of replacing function with arrow function

This makes this lexical. Is it OK here?

Thank you for your review.

I think that it is OK because test passed locally.
It is not called with neither .call nor .apply in this file.
This code equivalent to:

  return () => {
    return f.apply(null, args);
  };

Should I replace this with null ?

Maybe, for a clarity, but let's see what others think)

I got it.
Please let us know what others think.

The same.

Thank you for your review.
I was mistaken.

I'll fix this problem with like:

  toString() { return `${this.first} ${this.last}`; }

As per the comment above, it seems we should not change this fragment.

Modifications to them should be upstreamed first.

I'm sorry to have missed it.
I'll revert changes in this file.

Ditto.

Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime.

These tests are copied from WPT. I should not changed it directly.

I think that it should be reverted because function() {} and () => {} are should be tested both.
This PR changes only syntax, should not change test case.
So it better of separated by another PR.

What do you think about it ?

In my humble opinion, this check would be better to not replace arrow function. Because assert message also should have '[Function]' when function() {} is input.
We would be better to check both function style arrow function and normal function.

makeBlock does not need `this`. update `this` with `null` to clarify the intent.

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

`function() {}` and `() => {}` are should be tested both. See also #17345 (comment)

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

Hmm. It seems CI does not post status to PR currently.

Yesterday, status integration is worked fine.
What's happen ... ? 🤔

Hi @vsemozhetbyt.
Should I take any action to merge this PR ?

@Leko Sorry for the delay. Let's run the CI again. If it is green, we will land it today.

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

@vsemozhetbyt I got it. Thank you for quick reply !

One CI failure seems unrelated. Landing...

1. Among the list of Code and Learn, I solved the unfinished task of replacing function with arrow function: nodejs/code-and-learn#72 (comment) 2. Replace arrow function with shorter property syntax Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime. 3. Replace this with null makeBlock does not need `this`. update `this` with `null` to clarify the intent. PR-URL: #17345 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>

Squashed and landed in 3c62f33

Thank you, @Leko!

1. Among the list of Code and Learn, I solved the unfinished task of replacing function with arrow function: nodejs/code-and-learn#72 (comment) 2. Replace arrow function with shorter property syntax Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime. 3. Replace this with null makeBlock does not need `this`. update `this` with `null` to clarify the intent. PR-URL: #17345 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>

test: replace function with arrow function

79d1fbc

Among the list of [Code and Learn](nodejs/code-and-learn#72 (comment)), I solved the unfinished task of replacing function with arrow function

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

vsemozhetbyt added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 27, 2017

vsemozhetbyt reviewed Nov 27, 2017

View reviewed changes

Leko added 2 commits November 27, 2017 20:04

test: replace arrow function with shorter property syntax

c0e7ee8

Arrow function makes `this` lexical scope. But toString expects evaluate `this` in runtime.

test: revert changes in test-whatwg-url-searchparams-getall

6dccb02

These tests are copied from WPT. I should not changed it directly.

Leko commented Nov 27, 2017

View reviewed changes

yosuke-furukawa reviewed Nov 27, 2017

View reviewed changes

test: replace this with null

0739723

makeBlock does not need `this`. update `this` with `null` to clarify the intent.

mscdex added assert Issues and PRs related to the assert subsystem. domain Issues and PRs related to the domain subsystem. querystring Issues and PRs related to the built-in querystring module. labels Nov 27, 2017

test: revert to normal function

fa4eb72

`function() {}` and `() => {}` are should be tested both. See also #17345 (comment)

addaleax approved these changes Nov 27, 2017

View reviewed changes

maclover7 approved these changes Nov 27, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Nov 27, 2017

CI does not post status to PR currently #17358

Closed

addaleax approved these changes Nov 28, 2017

View reviewed changes

yosuke-furukawa approved these changes Nov 28, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Nov 29, 2017

CI status in PR differs from status in ci.nodejs.org #17387

Closed

vsemozhetbyt closed this Nov 29, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added dont-land-on-v6.x labels Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

               function makeBlock(f) {
                 const args = Array.prototype.slice.call(arguments, 1);
-                return function() {
+                return () => {

               // having an identical prototype property
               const nbRoot = {
-                toString: function() { return `${this.first} ${this.last}`; }
+                toString: () => { return `${this.first} ${this.last}`; }

               */
               /* eslint-disable */
-              test(function() {
+              test(() => {

               testAssertionMessage(/abc/gim, '/abc/gim');
               testAssertionMessage(function f() {}, '[Function: f]');
-              testAssertionMessage(function() {}, '[Function]');
+              testAssertionMessage(() => {}, '[Function]');

               testAssertionMessage(/abc/gim, '/abc/gim');
               testAssertionMessage(function f() {}, '[Function: f]');
-              testAssertionMessage(function() {}, '[Function]');
+              testAssertionMessage(() => {}, '[Function]');

test: replace function with arrow function by Leko · Pull Request #17345 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: replace function with arrow function - #17345

test: replace function with arrow function#17345Leko wants to merge 5 commits intonodejs:masternodejs/node:masterfrom Leko:replace_function_with_arrow_functionCopy head branch name to clipboard

Conversation

Leko commented Nov 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Nov 27, 2017

Uh oh!

vsemozhetbyt commented Nov 27, 2017

Uh oh!

vsemozhetbyt commented Nov 27, 2017

Uh oh!

Leko commented Nov 27, 2017

Uh oh!

Leko commented Nov 29, 2017

Uh oh!

vsemozhetbyt commented Nov 29, 2017

Uh oh!

Leko commented Nov 29, 2017

Uh oh!

vsemozhetbyt commented Nov 29, 2017

Uh oh!

vsemozhetbyt commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: replace function with arrow function#17345
Leko wants to merge 5 commits into
nodejs:masterfrom
Leko:replace_function_with_arrow_function

vsemozhetbyt Nov 27, 2017 •
edited
Loading