test: apply no-useless-concat ESLint rule #12613
Conversation
|
CI: https://ci.nodejs.org/job/node-test-pull-request/7624/ Only 1 Windows machine unstable (with flaky |
|
Is this commit supposed to modify an |
|
@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. |
|
@vsemozhetbyt +1 to adding the rule in this PR, seems no reason not to. |
|
@not-an-aardvark @gibfahn The rule is added. |
|
Still LGTM. |
| socket.write('HTTP/1.1 200 Connection Established' + '\r\n' + | ||
| 'Proxy-agent: Node-Proxy' + '\r\n' + | ||
| socket.write('HTTP/1.1 200 Connection Established\r\n' + | ||
| 'Proxy-agent: Node-Proxy\r\n' + |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It doesn't report multiline concatenations because people often use those intentionally (e.g. to avoid long lines).
Related: eslint/eslint#7947
There was a problem hiding this comment.
It just keeps strings in different lines:
Examples of correct code for this rule... when the string concatenation is multiline
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry, I was writing while your answer did not appear yet)
|
LGTM |
vsemozhetbyt
changed the title
|
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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
* Add `no-useless-concat: error` to .eslintrc.yaml. * Apply no-useless-concat rule to tests. PR-URL: #12613 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
* Add `no-useless-concat: error` to .eslintrc.yaml. * Apply no-useless-concat rule to tests. PR-URL: #12613 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
|
@vsemozhetbyt would you be willing to backport to v6.x? |
|
@gibfahn Sure. |
* 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test
It seems to me that no-useless-concat is a simple reasonable rule, so I've tried:
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.