Update: avoid creating gratuitous whitespace in arrow-body-style fixer

[not-an-aardvark]

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

[x] Changes an existing rule

What rule do you want to change?

arrow-body-style

Does this change cause the rule to produce more or fewer warnings?

The same amount

How will the change be implemented? (New option, new default behavior, etc.)?

This will change the default behavior.

Please provide some example code that this change will affect:

/* eslint arrow-body-style: "error" */

foo(bar => {
  return baz;
}, qux);

What does the rule currently do for this code?

It reports an error. With the --fix option, it fixes the code to:

foo(bar => 
   baz
, qux);

What will the rule do after it's changed?

It will still report an error. With the --fix option, it will fix the code to:

foo(bar => baz, qux);

This version of the fix is more idiomatic, and it avoids unexpected whitespace around the resulting expression.

What changes did you make? (Give an overview)

This updates the arrow-body-style fixer to remove whitespace when converting an arrow block body to an expression. It will only do this if there are no non-whitespace characters between the { and the return statement/between the expression and the }, so lines with comments will be unaffected.

Is there anything you'd like reviewers to focus on?

Nothing in particular.

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alberto, @vitorbal and @ajhyndman to be potential reviewers.

[eslintbot]

LGTM

[platinumazure]

Could you add some test cases for when the returned value is itself multi-line?

E.g.:

var x = () => {
    return foo
        .bar();
};

I'm wondering if maybe those cases shouldn't be fixed, but let's see how the test cases look with your changes.

[eslintbot]

LGTM

[not-an-aardvark]

Added some testcases. I think I like how the output for this case looks, but I don't like the output of this case as much.

[mysticatea]

FYI: A bit related: #6067

[kaicataldo]

@not-an-aardvark Agreed, but I think it's the safe approach. I don't think this fixer should take on the responsibility of formatting the returned value.

As an example:
Say the user has "newline-per-chained-call": "error" in their configuration and they run this fixer on the following code:

var foo = () => {
  return foo
    .bar()
    .baz();
};

is fixed to:

var foo = () => foo
    .bar()
    .baz();
};

The off indentation should be the responsibility of the indent rule, and the newline before the method call should be the responsibility of newline-per-chained-call. Since newline-per-chained-call doesn't have a fixer, fixing that to one line would result in a new errors.

[kaicataldo]

LGTM

[not-an-aardvark]

@platinumazure, what do you think about the added testcases for multiline expressions? It would be good to come to a consensus on how to handle those cases.

[platinumazure]

I think these are fairly sensible fixes. The CallExpression chain is a bit unfortunate, but I think tolerable given indent's autofixer. The ObjectExpression looks good to me (again, I would personally de-indent those lines but I know some are okay with a double-indent for an ObjectExpression inside parentheses). This is much better than what could otherwise have happened.

[platinumazure]

Sorry for dropping the ball on this. Not for the first time, I look forward to GitHub providing a way for me to track PRs which have had commits or history rewrites since my last review.

[not-an-aardvark]

Thanks! Regarding your comment about ObjectExpression indentation, keep in mind that this is also handled by indent's autofixer. (I couldn't tell whether you were just talking about the autofixer in the context of CallExpressions.)

Based on your comment, should I assume that you're 👍 on this proposal (in addition to approving the implementation)?

[platinumazure]

Yes, I'm :+1 on the proposal and implementation. (And yes, I know the indent autofixer handles both cases. Sorry for being unclear.)

[nzakas]

LGTM