Repository: eslint/eslint
Pull request #9149 (2017-08-23 18:17:29)

Update: make newline-per-chained-call fixable
**What is the purpose of this pull request? (put an "X" next to item)**

[ ] Documentation update
[ ] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[x] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

**What changes did you make? (Give an overview)**
This updates the [newline-per-chained-call](https://eslint.org/docs/rules/newline-per-chained-call) by making it fixable and does not affect the rule definition. The result of the fix was tested by adding the `output` according to the error messages.

The main motivation for this change is that code formatting tools like _prettier_ perform changes based on the line length, and since this rule is not fixable it warns of exceeding chained calls per line.

**Is there anything you'd like reviewers to focus on?**
No



eslintbot: LGTM
nzakas: Thanks for the pull request. Unfortunately, I think this is one of those rules that we can't support autofix for due to the complexities around calculating the correct indentation for each chain. Generally, we don't support autofix for rules that need to have information from other rules to end up 100% correct (we don't want to introduce additional linting errors when we autofix something).

I'll leave this open to see if any other team members believe I'm incorrect here.
not-an-aardvark: > Generally, we don't support autofix for rules that need to have information from other rules to end up 100% correct  (we don't want to introduce additional linting errors when we autofix something).

With other rules, I think we've generally decided that it's okay to do something like this because we now have multipass autofixing, so the new linting errors will be fixed on the next pass. For example, the autofixer for `quote-props` always adds double quotes around keys, which sometimes creates new `quotes` errors if the user prefers single quotes. However, this isn't a problem because the `quotes` autofixer will correct the quotes on the next pass.

We do a similar thing with the `brace-style` fixer:

```js
/* eslint brace-style: error, indent: error */

if (foo) {bar()}

// brace-style autofix ↓

if (foo) {
bar()
}

// indent autofix ↓

if (foo) {
    bar()
}
```

So in this case, I think it's okay if the `newline-per-chained-call` autofixer outputs lines with incorrect indentation, because the `indent` rule can fix it on the next pass. 
joaogranado: Thanks for your review. Initially, I had the same concern about indentation but as @not-an-aardvark said about multipass autofixing I really don't think this is actually an issue. This rule only concerns new lines on chain calls and  therefore it would only introduce conflicts between rules if we care about indentation. Many other _"newline"_ rules use the same approach so I think it is OK. Another point is that if we enforce indentation we could introduce conflicts with people that, for some reason, don't enforce indentation on chain calls. This particular rule conflicts with _prettier_, since it inlines everything that doesn't match the max-length limit, and considering that this rule is not fixable there is the need to use workarounds in order to remove the warnings.
nzakas: Good catch @not-an-aardvark. No further concerns from me. 
eslintbot: LGTM
joaogranado: @not-an-aardvark I've updated the code with the requested changes.
not-an-aardvark: cc @eslint/eslint-team -- anyone willing to support this enhancement?
eslintbot: LGTM
joaogranado: @kaicataldo I've updated the tests according to your comment. Good catch BTW! 😄
kaicataldo: Thanks! I'll champion this since we have 3 other supporters. This issue is now accepted.
joaogranado: Thanks!

Patch:
diff --git a/lib/rules/newline-per-chained-call.js b/lib/rules/newline-per-chained-call.js
index 613429d13..356c4baf3 100644
--- a/lib/rules/newline-per-chained-call.js
+++ b/lib/rules/newline-per-chained-call.js
@@ -19,7 +19,7 @@ module.exports = {
             category: "Stylistic Issues",
             recommended: false
         },
-
+        fixable: "whitespace",
         schema: [{
             type: "object",
             properties: {
@@ -40,6 +40,18 @@ module.exports = {
 
         const sourceCode = context.getSourceCode();
 
+        /**
+         * Get the prefix of a given MemberExpression node.
+         * If the MemberExpression node is a computed value it returns a
+         * left bracket. If not it returns a period.
+         *
+         * @param  {ASTNode} node - A MemberExpression node to get
+         * @returns {string} The prefix of the node.
+         */
+        function getPrefix(node) {
+            return node.computed ? "[" : ".";
+        }
+
         /**
          * Gets the property text of a given MemberExpression node.
          * If the text is multiline, this returns only the first line.
@@ -48,7 +60,7 @@ module.exports = {
          * @returns {string} The property text of the node.
          */
         function getPropertyText(node) {
-            const prefix = node.computed ? "[" : ".";
+            const prefix = getPrefix(node);
             const lines = sourceCode.getText(node.property).split(astUtils.LINEBREAK_MATCHER);
             const suffix = node.computed && lines.length === 1 ? "]" : "";
 
@@ -70,13 +82,18 @@ module.exports = {
                     parent = parent.callee.object;
                 }
 
-                if (depth > ignoreChainWithDepth && callee.property.loc.start.line === callee.object.loc.end.line) {
+                if (depth > ignoreChainWithDepth && astUtils.isTokenOnSameLine(callee.object, callee.property)) {
                     context.report({
                         node: callee.property,
                         loc: callee.property.loc.start,
                         message: "Expected line break before `{{callee}}`.",
                         data: {
                             callee: getPropertyText(callee)
+                        },
+                        fix(fixer) {
+                            const firstTokenAfterObject = sourceCode.getTokenAfter(callee.object, astUtils.isNotClosingParenToken);
+
+                            return fixer.insertTextBefore(firstTokenAfterObject, "\n");
                         }
                     });
                 }


Test patch:
diff --git a/tests/lib/rules/newline-per-chained-call.js b/tests/lib/rules/newline-per-chained-call.js
index 232aafed6..ada18a70b 100644
--- a/tests/lib/rules/newline-per-chained-call.js
+++ b/tests/lib/rules/newline-per-chained-call.js
@@ -28,6 +28,7 @@ ruleTester.run("newline-per-chained-call", rule, {
     }],
     invalid: [{
         code: "_\n.chain({}).map(foo).filter(bar).value();",
+        output: "_\n.chain({}).map(foo)\n.filter(bar)\n.value();",
         errors: [{
             message: "Expected line break before `.filter`."
         }, {
@@ -35,26 +36,31 @@ ruleTester.run("newline-per-chained-call", rule, {
         }]
     }, {
         code: "_\n.chain({})\n.map(foo)\n.filter(bar).value();",
+        output: "_\n.chain({})\n.map(foo)\n.filter(bar)\n.value();",
         errors: [{
             message: "Expected line break before `.value`."
         }]
     }, {
         code: "a().b().c().e.d()",
+        output: "a().b()\n.c().e.d()",
         errors: [{
             message: "Expected line break before `.c`."
         }]
     }, {
         code: "a.b.c().e().d()",
+        output: "a.b.c().e()\n.d()",
         errors: [{
             message: "Expected line break before `.d`."
         }]
     }, {
         code: "_.chain({}).map(a).value(); ",
+        output: "_.chain({}).map(a)\n.value(); ",
         errors: [{
             message: "Expected line break before `.value`."
         }]
     }, {
         code: "var a = m1.m2();\n var b = m1.m2().m3().m4().m5();",
+        output: "var a = m1.m2();\n var b = m1.m2().m3()\n.m4()\n.m5();",
         errors: [{
             message: "Expected line break before `.m4`."
         }, {
@@ -62,11 +68,13 @@ ruleTester.run("newline-per-chained-call", rule, {
         }]
     }, {
         code: "var a = m1.m2();\n var b = m1.m2().m3()\n.m4().m5();",
+        output: "var a = m1.m2();\n var b = m1.m2().m3()\n.m4()\n.m5();",
         errors: [{
             message: "Expected line break before `.m5`."
         }]
     }, {
         code: "var a = m1().m2\n.m3().m4().m5().m6().m7();",
+        output: "var a = m1().m2\n.m3().m4().m5()\n.m6()\n.m7();",
         options: [{
             ignoreChainWithDepth: 3
         }],
@@ -105,6 +113,37 @@ ruleTester.run("newline-per-chained-call", rule, {
             "    // Do something with error.",
             "}).end();"
         ].join("\n"),
+        output: [
+            "http.request({",
+            "    // Param",
+            "    // Param",
+            "    // Param",
+            "}).on('response', function(response) {",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "})",
+            ".on('error', function(error) {",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "    // Do something with error.",
+            "})",
+            ".end();"
+        ].join("\n"),
         errors: [{
             message: "Expected line break before `.on`."
         }, {
@@ -116,6 +155,13 @@ ruleTester.run("newline-per-chained-call", rule, {
             "    'method3' :",
             "    'method4']()"
         ].join("\n"),
+        output: [
+            "anObject.method1().method2()",
+            "['method' + n]()",
+            "[aCondition ?",
+            "    'method3' :",
+            "    'method4']()"
+        ].join("\n"),
         errors: [{
             message: "Expected line break before `['method' + n]`."
         }, {
@@ -123,8 +169,28 @@ ruleTester.run("newline-per-chained-call", rule, {
         }]
     }, {
         code: "foo.bar()['foo' + \u2029 + 'bar']()",
+        output: "foo.bar()\n['foo' + \u2029 + 'bar']()",
         options: [{ ignoreChainWithDepth: 1 }],
         errors: [{ message: "Expected line break before `['foo' + `." }]
+    }, {
+        code: "foo.bar()[(biz)]()",
+        output: "foo.bar()\n[(biz)]()",
+        options: [{ ignoreChainWithDepth: 1 }],
+        errors: [{ message: "Expected line break before `[biz]`." }]
+    }, {
+        code: "(foo).bar().biz()",
+        output: "(foo).bar()\n.biz()",
+        options: [{ ignoreChainWithDepth: 1 }],
+        errors: [{ message: "Expected line break before `.biz`." }]
+    }, {
+        code: "foo.bar(). /* comment */ biz()",
+        output: "foo.bar()\n. /* comment */ biz()",
+        options: [{ ignoreChainWithDepth: 1 }],
+        errors: [{ message: "Expected line break before `.biz`." }]
+    }, {
+        code: "foo.bar() /* comment */ .biz()",
+        output: "foo.bar() /* comment */ \n.biz()",
+        options: [{ ignoreChainWithDepth: 1 }],
+        errors: [{ message: "Expected line break before `.biz`." }]
     }]
-
 });