Update: make newline-per-chained-call fixable #9149

joaogranado · 2017-08-23T18:17:29Z

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)
[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 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 · 2017-08-23T18:17:30Z

LGTM

mention-bot · 2017-08-23T18:17:30Z

@joaogranado, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @rpatil26 and @vitorbal to be potential reviewers.

nzakas · 2017-08-23T19:03:59Z

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 · 2017-08-23T19:39:07Z

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:

/* 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 · 2017-08-23T20:32:59Z

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 on projects that (for some reason) don't enforce indentation on chain calls. This particular rule conflicts with prettier, since prettier 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 · 2017-08-25T20:22:07Z

Good catch @not-an-aardvark. No further concerns from me.

not-an-aardvark

Thanks for the pull request! This looks like a good start. I added a few suggestions.

not-an-aardvark · 2017-08-26T21:44:59Z

tests/lib/rules/newline-per-chained-call.js

        errors: [{
            message: "Expected line break before `['method' + n]`."
        }, {
            message: "Expected line break before `[aCondition ?`."
        }]
    }, {
        code: "foo.bar()['foo' + \u2029 + 'bar']()",
+        output: "foo.bar()\n['foo' + \u2029 + 'bar']()",


Can you add a test where the property is computed and parenthesized?

foo[(bar)]()

not-an-aardvark · 2017-08-26T21:47:15Z

tests/lib/rules/newline-per-chained-call.js

        errors: [{
            message: "Expected line break before `['method' + n]`."
        }, {
            message: "Expected line break before `[aCondition ?`."
        }]
    }, {
        code: "foo.bar()['foo' + \u2029 + 'bar']()",
+        output: "foo.bar()\n['foo' + \u2029 + 'bar']()",


Can you add a test where the object is parenthesized?

(foo).bar()

not-an-aardvark · 2017-08-26T21:49:51Z

tests/lib/rules/newline-per-chained-call.js

        errors: [{
            message: "Expected line break before `['method' + n]`."
        }, {
            message: "Expected line break before `[aCondition ?`."
        }]
    }, {
        code: "foo.bar()['foo' + \u2029 + 'bar']()",
+        output: "foo.bar()\n['foo' + \u2029 + 'bar']()",


Can you add a test where there is a comment between the object and the property?

foo . /* comment */ bar();

(I'm not really sure what output I would expect for this case since the code is sort of strange, so pretty much anything would be fine as long as it doesn't delete the comment and doesn't produce invalid syntax.)

You were right about the parenthesized properties/object, so I updated the fix() with your recommendation, and everything is passing accordingly. Regarding this comment, I would expect this to break before the . like:

foo . /* comment */ bar();

In my opinion the comment refers to the property, so it should break before the comment. Without further changes everything to the current implementation, it is working as expected. I added another test with the comment before the dot, and it breaks the line correctly as well:

foo /* comment */ .bar();

For what it's worth, in any case where we're unsure of the expected fix behavior, we can always choose not to fix. I think these cases where block comments make it difficult to infer what the user would really want would be a great place not to fix. Just my two cents.

If we opt to not fix it I think we need to change the rule in order to not report for block comments. The expected behaviour of the autofixer is to perform fixes, and if it leaves unfixed code after running it would break pre commit hooks, etc. There are other rules such as array-element-newline that fix it anyway like:

/* eslint array-element-newline: error */ const foo = [1,2 /* comment */,3]; // autofixes to: const foo = [1, 2 /* comment */, 3];

I don't feel strongly about this particular case, but there is precedent for autofixers not fixing all problems. In general we don't make any guarantees about whether a given autofix will be applied, and some rules aren't autofixable at all. For example, brace-style doesn't remove the linebreak before the { in cases like this:

if (foo) // comment { bar(); }

And eqeqeq can only autofix a small number of cases where a typeof expression or a literal is used.

But in the case of brace-style it is needed to get the final brace position since it would affect the final output. I think that in this case in this particular case it is ok to fix it anyway since it just fixes the code following the rule as it is, but it is up to you the final decision.

Seems reasonable to me.

not-an-aardvark · 2017-08-26T21:54:27Z

lib/rules/newline-per-chained-call.js

                    context.report({
                        node: callee.property,
                        loc: callee.property.loc.start,
                        message: "Expected line break before `{{callee}}`.",
                        data: {
                            callee: getPropertyText(callee)
+                        },
+                        fix(fixer) {
+                            return fixer.replaceTextRange([callee.object.range[1], callee.property.range[0]], `\n${getPrefix(callee)}`);


I think this will work incorrectly if the object or the property is parenthesized. callee.object.range doesn't include any surrounding parentheses, so for text like (foo).bar(), callee.object.range will be just the range of the foo token. Similarly, for text like foo[(bar)](), I think the current implementation will produce foo\n[bar)], which is invalid syntax.

You could avoid parentheses by doing something like this:

fix(fixer) { const firstTokenAfterObject = sourceCode.getTokenAfter(callee.object, astUtils.isNotClosingParenToken); return fixer.insertTextBefore(firstTokenAfterObject, "\n"); }

eslintbot · 2017-08-27T10:26:40Z

LGTM

joaogranado · 2017-08-27T10:36:23Z

@not-an-aardvark I've updated the code with the requested changes.

not-an-aardvark

Looks good to me. Thanks!

not-an-aardvark · 2017-09-10T20:28:26Z

cc @eslint/eslint-team -- anyone willing to support this enhancement?

kaicataldo

I support this enhancement. Just a few small comments, but this LGTM!

kaicataldo · 2017-09-10T21:50:27Z

tests/lib/rules/newline-per-chained-call.js

+            "    // Do something with response.",
+            "    // Do something with response.",
+            "    // Do something with response.",
+            "})\n.on('error', function(error) {",


Nitpick: Since we're actually writing this out with whitespace I think it would be clearer to add a new element to the array instead of inlining a \n character.

For example:

[ "})", ".on('error', function(error) {", ]

kaicataldo · 2017-09-10T21:51:16Z

tests/lib/rules/newline-per-chained-call.js

@@ -116,15 +153,40 @@ ruleTester.run("newline-per-chained-call", rule, {
            "    'method3' :",
            "    'method4']()"
        ].join("\n"),
+        output: [
+            "anObject.method1().method2()\n['method' + n]()\n[aCondition ?",


Nitpick: Since we're actually writing this out with whitespace I think it would be clearer to add a new element to the array instead of inlining a \n character.

eslintbot · 2017-09-12T19:36:55Z

LGTM

joaogranado · 2017-09-12T19:40:04Z

@kaicataldo I've updated the tests according to your comment. Good catch BTW! 😄

kaicataldo · 2017-09-12T19:54:05Z

Thanks! I'll champion this since we have 3 other supporters. This issue is now accepted.

kaicataldo

LGTM. Thanks for contributing to ESLint!

Leaving this open for another day since it was just accepted.

joaogranado · 2017-09-12T20:13:15Z

Thanks!

kaicataldo · 2017-09-13T18:14:05Z

Thanks for contributing to ESLint!

not-an-aardvark added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Aug 23, 2017

not-an-aardvark suggested changes Aug 26, 2017

View reviewed changes

joaogranado force-pushed the fixable-newline-per-chained-call branch from b4eabdb to ccaf1b9 Compare August 27, 2017 10:26

not-an-aardvark approved these changes Sep 1, 2017

View reviewed changes

kaicataldo approved these changes Sep 10, 2017

View reviewed changes

Update: make newline-per-chained-call fixable

eb1365f

joaogranado force-pushed the fixable-newline-per-chained-call branch from ccaf1b9 to eb1365f Compare September 12, 2017 19:36

kaicataldo self-assigned this Sep 12, 2017

kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 12, 2017

kaicataldo approved these changes Sep 12, 2017

View reviewed changes

kaicataldo merged commit 2731f94 into eslint:master Sep 13, 2017

eslint-deprecated bot locked and limited conversation to collaborators Mar 13, 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 Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: make newline-per-chained-call fixable #9149

Update: make newline-per-chained-call fixable #9149

joaogranado commented Aug 23, 2017

eslintbot commented Aug 23, 2017

mention-bot commented Aug 23, 2017

nzakas commented Aug 23, 2017

not-an-aardvark commented Aug 23, 2017

joaogranado commented Aug 23, 2017 •

edited

nzakas commented Aug 25, 2017

not-an-aardvark left a comment

not-an-aardvark Aug 26, 2017

not-an-aardvark Aug 26, 2017

not-an-aardvark Aug 26, 2017

joaogranado Aug 27, 2017

platinumazure Aug 27, 2017

joaogranado Aug 27, 2017

not-an-aardvark Aug 27, 2017

joaogranado Aug 28, 2017 •

edited

not-an-aardvark Aug 28, 2017

not-an-aardvark Aug 26, 2017

eslintbot commented Aug 27, 2017

joaogranado commented Aug 27, 2017

not-an-aardvark left a comment

not-an-aardvark commented Sep 10, 2017

kaicataldo left a comment

kaicataldo Sep 10, 2017

kaicataldo Sep 10, 2017

eslintbot commented Sep 12, 2017

joaogranado commented Sep 12, 2017

kaicataldo commented Sep 12, 2017

kaicataldo left a comment •

edited

joaogranado commented Sep 12, 2017

kaicataldo commented Sep 13, 2017

Update: make newline-per-chained-call fixable #9149

Update: make newline-per-chained-call fixable #9149

Conversation

joaogranado commented Aug 23, 2017

eslintbot commented Aug 23, 2017

mention-bot commented Aug 23, 2017

nzakas commented Aug 23, 2017

not-an-aardvark commented Aug 23, 2017

joaogranado commented Aug 23, 2017 • edited

nzakas commented Aug 25, 2017

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

joaogranado Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 27, 2017

joaogranado commented Aug 27, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Sep 10, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 12, 2017

joaogranado commented Sep 12, 2017

kaicataldo commented Sep 12, 2017

kaicataldo left a comment • edited

Choose a reason for hiding this comment

joaogranado commented Sep 12, 2017

kaicataldo commented Sep 13, 2017

joaogranado commented Aug 23, 2017 •

edited

joaogranado Aug 28, 2017 •

edited

kaicataldo left a comment •

edited