Chore: Enable `no-useless-escape` #7403

vitorbal · 2016-10-19T16:41:46Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Chore, see overview section below.

What changes did you make? (Give an overview)
As per the discussion here, I've turned the no-useless-escape rule on and fixed all the pertinent errors in our codebase.

Is there anything you'd like reviewers to focus on?
Nothing in specific, I guess just some extra eyes to make sure there were not any false-positives.

eslintbot · 2016-10-19T16:41:48Z

LGTM

mention-bot · 2016-10-19T16:41:50Z

@vitorbal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Kyle-Mendes, @azhang496 and @kaicataldo to be potential reviewers.

kaicataldo · 2016-10-19T16:50:26Z

Looks like there's a linting error

not-an-aardvark

Thanks! This mostly looks good, I just have a few questions about the changes.

not-an-aardvark · 2016-10-19T17:26:37Z

tests/lib/rules/no-useless-escape.js

@@ -111,7 +111,7 @@ ruleTester.run("no-useless-escape", rule, {
        { code: "var foo = `\\'`;", parserOptions: {ecmaVersion: 6}, errors: [{ line: 1, column: 11, message: "Unnecessary escape character: \\'.", type: "TemplateElement"}] },
        { code: "var foo = `\\#`;", parserOptions: {ecmaVersion: 6}, errors: [{ line: 1, column: 11, message: "Unnecessary escape character: \\#.", type: "TemplateElement"}] },
        {
-            code: "var foo = '\\\`foo\\\`';",
+            code: "var foo = '\\\`foo\\\`';", // eslint-disable-line no-useless-escape


Could this line be replaced with:

code: "var foo = '\\`foo\\`';",

I'm not sure I understand why disabling the rule on this line is necessary.

Thanks, you're totally right! Think I thought the inner one was a single-quote too. Should use my slightly larger glasses next time :)

not-an-aardvark · 2016-10-19T17:26:41Z

tests/lib/rules/comma-dangle.js

@@ -271,7 +271,7 @@ ruleTester.run("comma-dangle", rule, {
            options: ["always-multiline"],
        },
        {
-            code: "foo(\na,\mb\m)",


Is this supposed to be something different? Removing only the useless escapes would result in

code: "foo(\na,mbm)",

But I'm not sure what the intention of the test is.

I'm guessing typo (i.e., should be "foo(\na,\nb\n)".

Yeah, I was not sure about this one, as the test above this one is also testing "foo(\na,\nb\n)". I might as well just delete this one?

Actually, might as well leave the new version, as there's no other test that tests foo(\na,b) (as in, newline after opening parenthesis, no newline after first argument)

not-an-aardvark · 2016-10-19T17:26:50Z

tests/lib/rules/no-useless-escape.js

@@ -126,7 +126,7 @@ ruleTester.run("no-useless-escape", rule, {
            ]
        },
        {
-            code: "var foo = `\\\'${foo}\\\'`;",
+            code: "var foo = `\\\'${foo}\\\'`;", // eslint-disable-line no-useless-escape


Same thing here

platinumazure

I'm fairly sure all of the inline disables can be resolved, as long as we can understand what the original intent of each test was. Happy hunting! 😄

platinumazure · 2016-10-19T17:39:09Z

tests/lib/rules/new-cap.js

@@ -62,10 +62,12 @@ ruleTester.run("new-cap", rule, {

        { code: "var x = Foo.Bar(42);", options: [{ capIsNewExceptions: ["Bar"] }] },
        { code: "var x = Foo.Bar(42);", options: [{ capIsNewExceptions: ["Foo.Bar"] }] },
-        { code: "var x = Foo.Bar(42);", options: [{ capIsNewExceptionPattern: "^Foo\.." }] },
+
+        { code: "var x = Foo.Bar(42);", options: [{ capIsNewExceptionPattern: "^Foo\.." }] }, // eslint-disable-line no-useless-escape


Pretty sure this should be capIsNewExceptionPattern: "^Foo\\.." (value is a string that we want interpreted as a regex, so we need to escape the backslash here).

This test is probably passing by accident because \. is generously interpreted as ., so the real pattern is Foo.., and "Foo.B" does match that pattern.

You are totally right, good catch! Thank you :)

platinumazure · 2016-10-19T17:39:19Z

tests/lib/rules/new-cap.js

        { code: "var x = new foo.bar(42);", options: [{ newIsCapExceptions: ["bar"] }] },
        { code: "var x = new foo.bar(42);", options: [{ newIsCapExceptions: ["foo.bar"] }] },
-        { code: "var x = new foo.bar(42);", options: [{ newIsCapExceptionPattern: "^foo\.." }] },
+
+        { code: "var x = new foo.bar(42);", options: [{ newIsCapExceptionPattern: "^foo\.." }] }, // eslint-disable-line no-useless-escape


platinumazure · 2016-10-19T17:39:39Z

tests/lib/rules/new-cap.js

@@ -143,7 +145,8 @@ ruleTester.run("new-cap", rule, {
        },
        {
            code: "var x = Bar.Foo(42);",
-            options: [{capIsNewExceptionPattern: "^Foo\.."}],
+
+            options: [{capIsNewExceptionPattern: "^Foo\.."}], // eslint-disable-line no-useless-escape


Same here, although in this case it matters less since the test fails anyway.

platinumazure · 2016-10-19T17:39:55Z

tests/lib/rules/new-cap.js

@@ -153,7 +156,8 @@ ruleTester.run("new-cap", rule, {
        },
        {
            code: "var x = new bar.foo(42);",
-            options: [{newIsCapExceptionPattern: "^foo\.."}],
+
+            options: [{newIsCapExceptionPattern: "^foo\.."}], // eslint-disable-line no-useless-escape


And here as well.

platinumazure · 2016-10-19T17:41:15Z

tests/lib/rules/no-invalid-regexp.js

@@ -26,11 +26,11 @@ ruleTester.run("no-invalid-regexp", rule, {
        { code: "new RegExp('.', 'y')", options: [{ allowConstructorFlags: ["y"] }]},
        { code: "new RegExp('.', 'u')", options: [{ allowConstructorFlags: ["U"] }]},
        { code: "new RegExp('.', 'yu')", options: [{ allowConstructorFlags: ["y", "u"] }]},
-        { code: "new RegExp('\/', 'yu')", options: [{ allowConstructorFlags: ["y", "u"] }]},
+        { code: "new RegExp('\/', 'yu')", options: [{ allowConstructorFlags: ["y", "u"] }]}, // eslint-disable-line no-useless-escape


I think we need to figure out the intent of this test. Either the regex is /\\\// (matching backslash followed by slash), in which case this string should say '\\/'; or the regex is /\// (matching slash), in which case the string should say '/'.

I honestly cannot tell why is this test different than the one on the line above it. Here's the original commit: c0a7be6#diff-9f7fae656233bad860f2ba8d46986835R26
Those tests were added to test the new y and u flags, as it seems.

Both of them resolve to the same regexp literal in the end, so i'll change this to '/'.

platinumazure · 2016-10-19T17:41:25Z

tests/lib/rules/no-invalid-regexp.js

        { code: "new RegExp('.', 'y')", parserOptions: { ecmaVersion: 6 }},
        { code: "new RegExp('.', 'u')", parserOptions: { ecmaVersion: 6 }},
        { code: "new RegExp('.', 'yu')", parserOptions: { ecmaVersion: 6 }},
-        { code: "new RegExp('\/', 'yu')", parserOptions: { ecmaVersion: 6 }}
+        { code: "new RegExp('\/', 'yu')", parserOptions: { ecmaVersion: 6 }} // eslint-disable-line no-useless-escape


platinumazure · 2016-10-19T17:42:45Z

tests/lib/rules/no-useless-escape.js

@@ -72,8 +72,8 @@ ruleTester.run("no-useless-escape", rule, {
        {code: "var foo = `${foo}\\f`;", parserOptions: {ecmaVersion: 6}},
        {code: "var foo = `${foo}\\\n`;", parserOptions: {ecmaVersion: 6}},
        {code: "var foo = `${foo}\\\r\n`;", parserOptions: {ecmaVersion: 6}},
-        {code: "var foo = `\\\``", parserOptions: {ecmaVersion: 6}},
-        {code: "var foo = `\\\`${foo}\\\``", parserOptions: {ecmaVersion: 6}},
+        {code: "var foo = `\\\``", parserOptions: {ecmaVersion: 6}}, // eslint-disable-line no-useless-escape


Not sure what the intent of this test is either, but again we should be able to resolve this as either two backslashes total or four.

4 backslashes would produce a parsing error in this case, so I will resolve this one to two backslashes.

platinumazure · 2016-10-19T17:43:04Z

tests/lib/rules/no-useless-escape.js

-        {code: "var foo = `\\\``", parserOptions: {ecmaVersion: 6}},
-        {code: "var foo = `\\\`${foo}\\\``", parserOptions: {ecmaVersion: 6}},
+        {code: "var foo = `\\\``", parserOptions: {ecmaVersion: 6}}, // eslint-disable-line no-useless-escape
+        {code: "var foo = `\\\`${foo}\\\``", parserOptions: {ecmaVersion: 6}}, // eslint-disable-line no-useless-escape


And probably here as well (albeit with more total backslashes).

Similar here, I'll resolve this one to two backslashes. Thanks for all the feedback!

not-an-aardvark · 2016-10-19T18:17:18Z

All this confusion on the intended behavior is making me think we should add no-useless-escape to eslint:recommended next major release 😄

eslintbot · 2016-10-20T16:16:07Z

LGTM

platinumazure

LGTM, thanks!

Chore: Enable no-useless-escape

9a5cfc6

jquerybot added the CLA: Valid label Oct 19, 2016

not-an-aardvark reviewed Oct 19, 2016

View reviewed changes

platinumazure requested changes Oct 19, 2016

View reviewed changes

Chore: Fix linting issues

8a627da

vitorbal added the chore This change is not user-facing label Oct 20, 2016

Chore: clean up eslint-disable comments

b46b71c

platinumazure approved these changes Oct 20, 2016

View reviewed changes

not-an-aardvark approved these changes Oct 20, 2016

View reviewed changes

nzakas merged commit 18ff70f into master Oct 21, 2016

vitorbal deleted the no-useless-escape branch October 21, 2016 23:26

popomore mentioned this pull request Nov 1, 2016

feat: add getLogger to app and ctx eggjs/egg#136

Merged

3 tasks

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Enable `no-useless-escape` #7403

Chore: Enable `no-useless-escape` #7403

vitorbal commented Oct 19, 2016

eslintbot commented Oct 19, 2016

mention-bot commented Oct 19, 2016

kaicataldo commented Oct 19, 2016

not-an-aardvark left a comment

not-an-aardvark Oct 19, 2016

vitorbal Oct 20, 2016

not-an-aardvark Oct 19, 2016

platinumazure Oct 19, 2016

vitorbal Oct 20, 2016

vitorbal Oct 20, 2016

not-an-aardvark Oct 19, 2016

platinumazure left a comment

platinumazure Oct 19, 2016

vitorbal Oct 20, 2016

platinumazure Oct 19, 2016

platinumazure Oct 19, 2016

platinumazure Oct 19, 2016

platinumazure Oct 19, 2016 •

edited

vitorbal Oct 20, 2016 •

edited

vitorbal Oct 20, 2016

platinumazure Oct 19, 2016

platinumazure Oct 19, 2016

vitorbal Oct 20, 2016

platinumazure Oct 19, 2016 •

edited

vitorbal Oct 20, 2016

not-an-aardvark commented Oct 19, 2016

eslintbot commented Oct 20, 2016

platinumazure left a comment

Chore: Enable no-useless-escape #7403

Chore: Enable no-useless-escape #7403

Conversation

vitorbal commented Oct 19, 2016

eslintbot commented Oct 19, 2016

mention-bot commented Oct 19, 2016

kaicataldo commented Oct 19, 2016

not-an-aardvark 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

platinumazure 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

platinumazure Oct 19, 2016 • edited

Choose a reason for hiding this comment

vitorbal Oct 20, 2016 • edited

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

platinumazure Oct 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Oct 19, 2016

eslintbot commented Oct 20, 2016

platinumazure left a comment

Choose a reason for hiding this comment

Chore: Enable `no-useless-escape` #7403

Chore: Enable `no-useless-escape` #7403

platinumazure Oct 19, 2016 •

edited

vitorbal Oct 20, 2016 •

edited

platinumazure Oct 19, 2016 •

edited