test: apply no-useless-concat ESLint rule by vsemozhetbyt · Pull Request #12613 · nodejs/node

vsemozhetbyt · 2017-04-23T22:30:57Z

Checklist

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

Affected core subsystem(s)

test

It seems to me that no-useless-concat is a simple reasonable rule, so I've tried:

eslint --rulesdir tools/eslint-rules/ --rule "no-useless-concat: error" benchmark lib test tools

which have found only some fragments in tests. Occasionally, I've added some template literals for simplicity, but that was not the main aim. Please, take a look, if this is a reasonable change.

vsemozhetbyt · 2017-04-23T22:32:06Z

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

Only 1 Windows machine unstable (with flaky sequential/test-benchmark-child-process).

gibfahn · 2017-04-24T07:30:19Z

cc/ @not-an-aardvark, @silverwind, @Trott @mscdex

not-an-aardvark · 2017-04-24T07:41:23Z

Is this commit supposed to modify an .eslintrc.yaml file? I see changed code but it doesn't look like any ESLint rules were actually enabled.

vsemozhetbyt · 2017-04-24T08:09:16Z

@not-an-aardvark No, I did not mean to propose to enable the rule, just to correct the code itself. However, if there is a consensus to add the rule, this can be done as well (here or in an other PR), just let me know.

gibfahn · 2017-04-24T09:08:28Z

@vsemozhetbyt +1 to adding the rule in this PR, seems no reason not to.

vsemozhetbyt · 2017-04-24T09:25:05Z

@not-an-aardvark @gibfahn The rule is added.

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

gibfahn · 2017-04-24T09:38:10Z

cc/ @refack @addaleax @lpinca , does this still LGTY with the lint rule added?

lpinca · 2017-04-24T10:58:15Z

Still LGTM.

mscdex · 2017-04-24T16:39:58Z

test/parallel/test-http-server-unconsume-consume.js

I wonder why the new eslint rule didn't complain about the next '\r\n' not being included as well? I guess it only considers string literals that are in between two other literals?

It doesn't report multiline concatenations because people often use those intentionally (e.g. to avoid long lines).

Related: eslint/eslint#7947

It just keeps strings in different lines:

Examples of correct code for this rule... when the string concatenation is multiline

~~Yes, and that's why it didn't complain about the next '\r\n'.~~

~~I think we're both saying the same thing -- did I misunderstand the question?~~

edit: Never mind, I didn't read the author of the last reply and thought it was from @mscdex.

Sorry, I was writing while your answer did not appear yet)

mscdex · 2017-04-24T16:47:34Z

LGTM

vsemozhetbyt · 2017-04-25T19:33:13Z

Landed in 71abfa9

* Add `no-useless-concat: error` to .eslintrc.yaml. * Apply no-useless-concat rule to tests. PR-URL: #12613 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

gibfahn · 2017-05-16T15:33:36Z

@vsemozhetbyt would you be willing to backport to v6.x?

vsemozhetbyt · 2017-05-16T15:42:04Z

@gibfahn Sure.

vsemozhetbyt · 2017-05-16T16:04:44Z

@gibfahn PTAL: #13056

* Add `no-useless-concat: error` to .eslintrc.yaml. * Apply no-useless-concat rule to tests. PR-URL: #12613 Backport-PR-URL: #13056 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

* Add `no-useless-concat: error` to .eslintrc.yaml. * Apply no-useless-concat rule to tests. PR-URL: nodejs/node#12613 Backport-PR-URL: nodejs/node#13056 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

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

addaleax approved these changes Apr 24, 2017

View reviewed changes

refack approved these changes Apr 24, 2017

View reviewed changes

lpinca approved these changes Apr 24, 2017

View reviewed changes

gibfahn approved these changes Apr 24, 2017

View reviewed changes

cjihrig approved these changes Apr 24, 2017

View reviewed changes

not-an-aardvark approved these changes Apr 24, 2017

View reviewed changes

mscdex reviewed Apr 24, 2017

View reviewed changes

jasnell approved these changes Apr 24, 2017

View reviewed changes

vsemozhetbyt changed the title ~~test: apply no-useless-concat ESLint rule to tests~~ test: apply no-useless-concat ESLint rule Apr 24, 2017

vsemozhetbyt added 2 commits April 25, 2017 22:23

test: apply no-useless-concat ESLint rule to tests

3f938c7

tools: add no-useless-concat: error to .eslintrc

aa92c15

vsemozhetbyt closed this Apr 25, 2017

vsemozhetbyt deleted the no-useless-concat branch April 25, 2017 19:33

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added backport-requested-v6.x labels May 16, 2017

vsemozhetbyt mentioned this pull request May 16, 2017

(v6.x backport) tools: use no-useless-concat ESLint rule #13056

Closed

2 tasks

gibfahn added the backported-to-v6.x label May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Uh oh!

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

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

Uh oh!

Uh oh!

gibfahn commented Apr 24, 2017

Uh oh!

not-an-aardvark commented Apr 24, 2017

Uh oh!

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

Uh oh!

Uh oh!

gibfahn commented Apr 24, 2017

Uh oh!

vsemozhetbyt commented Apr 24, 2017

Uh oh!

gibfahn commented Apr 24, 2017

Uh oh!

lpinca commented Apr 24, 2017

Uh oh!

mscdex Apr 24, 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!

not-an-aardvark Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 24, 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!

not-an-aardvark Apr 24, 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!

vsemozhetbyt Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex commented Apr 24, 2017

Uh oh!

vsemozhetbyt commented Apr 25, 2017

Uh oh!

gibfahn commented May 16, 2017

Uh oh!

vsemozhetbyt commented May 16, 2017

Uh oh!

vsemozhetbyt commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

vsemozhetbyt commented Apr 23, 2017 •

edited

Loading

vsemozhetbyt commented Apr 23, 2017 •

edited

Loading

vsemozhetbyt commented Apr 24, 2017 •

edited

Loading

mscdex Apr 24, 2017 •

edited

Loading

vsemozhetbyt Apr 24, 2017 •

edited

Loading

not-an-aardvark Apr 24, 2017 •

edited

Loading