tools: add 'spaced-comment' into eslint rules #19596

starkwang · 2018-03-25T14:37:28Z

This rule is not very important but still makes sense to some first-time PRs.

Checklist

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

vsemozhetbyt · 2018-03-25T15:03:08Z

test/parallel/test-crypto.js

@@ -260,7 +260,7 @@ assert.throws(function() {

 /**


Is this line correct now?

Yes it's correct: https://eslint.org/docs/rules/spaced-comment#always

Is this some built-in exception for jsdoc?

Yes. '*' is the default marker (see here), so /** can pass the check.

vsemozhetbyt · 2018-03-25T15:06:27Z

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

BridgeAR

I like this. Just a couple suggestions to remove things that seem to be obsolete. When being on it: it would also be very nice if you would use a capital letter for the first character of a comment :-) that way these changes have to be done only once instead of twice.

BridgeAR · 2018-03-25T15:13:32Z

test/parallel/test-tls-server-verify.js

@@ -227,10 +227,10 @@ function runClient(prefix, port, options, cb) {
    }
  });

-  //client.stdout.pipe(process.stdout);
+  // client.stdout.pipe(process.stdout);


Suggestion: remove this. There is no comment about why this is kept in here.

BridgeAR · 2018-03-25T15:13:47Z

test/parallel/test-tls-server-verify.js


  client.on('exit', function(code) {
-    //assert.strictEqual(
+    // assert.strictEqual(
    //  0, code,
    //  `${prefix}${options.name}: s_client exited with error code ${code}`);


Suggestion: remove this. There is no comment about why this is kept in here.

BridgeAR · 2018-03-25T15:14:20Z

test/pummel/test-https-ci-reneg-attack.js

-    //child.stdout.pipe(process.stdout);
-    //child.stderr.pipe(process.stderr);
+    // child.stdout.pipe(process.stdout);
+    // child.stderr.pipe(process.stderr);


Suggestion: remove this. There is no comment why this is kept in here.

BridgeAR · 2018-03-25T15:15:02Z

test/pummel/test-regress-GH-814.js

@@ -32,7 +32,7 @@ function newBuffer(size, value) {
  while (size--) {
    buffer[size] = value;
  }
-  //buffer[buffer.length-2]= 0x0d;
+  // buffer[buffer.length-2]= 0x0d;


Suggestion: remove this. There is no comment why this is kept in here.

BridgeAR · 2018-03-25T15:15:47Z

test/pummel/test-regress-GH-814_2.js

@@ -39,7 +39,7 @@ function tailCB(data) {
  PASS = !data.toString().includes('.');

  if (PASS) {
-    //console.error('i');
+    // console.error('i');
  } else {


Suggestion: rewrite this to if (!PASS) { ...

BridgeAR · 2018-03-25T15:18:58Z

test/parallel/test-https-host-headers.js

@@ -55,7 +55,7 @@ function testHttps() {
      method: 'GET',
      path: `/${counter++}`,
      host: 'localhost',
-      //agent: false,
+      // agent: false,


Suggestion: remove this and the other cases as well. There is no comment why this is kept in here.

BridgeAR · 2018-03-25T15:20:25Z

test/parallel/test-http-set-trailers.js

@@ -78,7 +78,7 @@ server.on('listening', function() {
  });

  c.on('data', function(chunk) {
-    //console.log(chunk);
+    // console.log(chunk);


Suggestion: remove this. If the test fails, this has to be debugged properly anyway and maybe needs a code change but the comment does not help in this case.

BridgeAR · 2018-03-25T15:21:19Z

test/parallel/test-http-set-trailers.js

@@ -103,7 +103,7 @@ server.on('listening', function() {
    headers: {}
  }, function(res) {
    res.on('end', function() {
-      //console.log(res.trailers);
+      // console.log(res.trailers);
      assert.ok('x-foo' in res.trailers, 'Client doesn\'t see trailers.');


Suggestion: remove the comment and change the error message to include res.trailers.

BridgeAR · 2018-03-25T15:22:51Z

test/parallel/test-http-client-timeout-agent.js

@@ -32,7 +32,7 @@ const options = {
  host: '127.0.0.1',
 };

-//http.globalAgent.maxSockets = 15;
+// http.globalAgent.maxSockets = 15;


Suggestion: remove this. There is no comment why this is kept in here.

BridgeAR · 2018-03-25T15:23:52Z

test/parallel/test-http-allow-req-after-204-res.js

@@ -49,7 +49,7 @@ function nextRequest() {
        // throws error:
        nextRequest();
        // works just fine:
-        //process.nextTick(nextRequest);
+        // process.nextTick(nextRequest);


Please add TODO: investigate why this does not work fine even though it should..

starkwang · 2018-03-25T16:11:50Z

test/parallel/test-http-set-trailers.js

-      //console.log(res.trailers);
-      assert.ok('x-foo' in res.trailers, 'Client doesn\'t see trailers.');
+      assert.ok('x-foo' in res.trailers,
+                'Client doesn\'t see trailers in res.trailers');


res.trailers is an Object. Should we use the Object.keys to show all the keys? @BridgeAR

Maybe just use util.inspect? Object.keys works just as fine as well though, as it is only about the keys in this case.

starkwang · 2018-03-25T16:28:00Z

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

thefourtheye

I would prefer landing the commented code removal as a separate commit.

Trott · 2018-03-26T20:54:33Z

I suspect some on @nodejs/release might greatly prefer that the backports for this be in place when this lands so they can land immediately too?

On the other hand, maybe not since it's only affecting comments?

I'll add backport-requested labels but feel free to remove them if that's not the right thing to do.

Trott · 2018-03-26T20:55:09Z

(Left out a backport request for 4.x since it's EOL in about a month.)

starkwang · 2018-04-01T13:21:42Z

CI before land: https://ci.nodejs.org/job/node-test-pull-request/13973/

starkwang · 2018-04-01T14:35:01Z

Landed in 2540581

PR-URL: #19596 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#19596 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 25, 2018

starkwang added the tools Issues and PRs related to the tools directory. label Mar 25, 2018

vsemozhetbyt reviewed Mar 25, 2018

View reviewed changes

vsemozhetbyt approved these changes Mar 25, 2018

View reviewed changes

BridgeAR approved these changes Mar 25, 2018

View reviewed changes

starkwang force-pushed the add-eslint-spaced-comment branch from f5d2b2a to 426ff47 Compare March 25, 2018 15:46

trivikr approved these changes Mar 25, 2018

View reviewed changes

starkwang commented Mar 25, 2018

View reviewed changes

thefourtheye approved these changes Mar 26, 2018

View reviewed changes

Trott added backport-requested-v6.x labels Mar 26, 2018

starkwang force-pushed the add-eslint-spaced-comment branch from b2e2078 to 2d57d02 Compare April 1, 2018 13:14

starkwang added 5 commits April 1, 2018 21:19

tools: add 'spaced-comment' into eslint rules

fd72185

[squash] remove obsolete comments

c39bae5

[squash] capitalize the first letter

262d32f

[squash] change assert message

e705987

fix

e06a3ec

starkwang force-pushed the add-eslint-spaced-comment branch from 2d57d02 to e06a3ec Compare April 1, 2018 13:19

starkwang closed this Apr 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add 'spaced-comment' into eslint rules #19596

tools: add 'spaced-comment' into eslint rules #19596

starkwang commented Mar 25, 2018 •

edited

Loading

vsemozhetbyt Mar 25, 2018

starkwang Mar 25, 2018

vsemozhetbyt Mar 25, 2018

starkwang Mar 25, 2018

vsemozhetbyt commented Mar 25, 2018

BridgeAR left a comment

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

BridgeAR Mar 25, 2018

starkwang Mar 25, 2018 •

edited

Loading

BridgeAR Mar 25, 2018

starkwang commented Mar 25, 2018

thefourtheye left a comment

Trott commented Mar 26, 2018

Trott commented Mar 26, 2018

starkwang commented Apr 1, 2018

starkwang commented Apr 1, 2018

tools: add 'spaced-comment' into eslint rules #19596

tools: add 'spaced-comment' into eslint rules #19596

Conversation

starkwang commented Mar 25, 2018 • edited Loading

Checklist

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

vsemozhetbyt commented Mar 25, 2018

BridgeAR left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkwang Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkwang commented Mar 25, 2018

thefourtheye left a comment

Choose a reason for hiding this comment

Trott commented Mar 26, 2018

Trott commented Mar 26, 2018

starkwang commented Apr 1, 2018

starkwang commented Apr 1, 2018

starkwang commented Mar 25, 2018 •

edited

Loading

starkwang Mar 25, 2018 •

edited

Loading