Fix: Account for comments in implicit-arrow-linebreak

[ghost]

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

[ ] Documentation update
[X ] 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
[ ] Other, please explain:

Implicit-arrow-linebreak does not take comments into consideration when the option for the rule is beside.

When the rule evaluates the expression:

(foo) =>
   //comment
   bar;

The outcome is:

(foo) => bar;

The proposed fix is:

// comment
(foo) => bar;

What changes did you make? (Give an overview)

• Add test cases including comments in tests/lib/rules/implicit-arrow-linebreak
• Add helper for formatting comment text split by line breaks
• Implement filtering condition, and fix for when the above bug occurs

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

• This is my first PR for this project -- any feedback would be heavily appreciated.
• Do the test cases suffice? Any missing logic?
• What should be the outcome with the test case of

(foo) =>
  // hi
     bar =>
       // there
         braz;

? This one was a doozy.
@sharmilajesupaul

[jsf-clabot]

All committers have signed the CLA.

[ilyavolodin]

Hmm... It seems like this fix is improving current behavior, but I'm not 100% sure this is the best way. In the last example you showed, the only reasonable way to fix it based on your approach would be to move all of the comments above the line:

// hi
// there
(foo) =>bar => braz;

However, chances are, if there's a chaining like that, comments describe what each step of the chain is doing, and if you move them above, it would be unclear. I think best way to fix that would be to change line comments into inline comments.

(foo) => /* hi */ bar => /* there */ braz;

But that might not work and might possibly change the meaning of the code. So I'm not sure what would be the best way to do that.

[not-an-aardvark]

I think the best solution would be to just not do an autofix if a line comment is found in that location. We have a similar behavior with brace-style, where we don't move the opening bracket when the code looks like this:

if (foo) // comment
{
}

[ljharb]

I'd expect:

(foo) =>
  // hi
     bar =>
       // there
         braz;

to become:

(foo) => (
  // hi
     bar => (
       // there
         braz
      )
);

[ghost]

@ljharb @not-an-aardvark @ilyavolodin Thanks for all of your input. It seems like prepending the arrow functions with parentheses clarifies the intent of the comments.

I will go ahead and implement this change.

[sharmilajesupaul]

Looks good 👍 thanks for fixing this bug!

[ghost]

@sharmilajesupaul Thanks for the review and comments! Still working on the above test case as well as when:

async (foo) => 
    // comment
    bar

Should be finished by tomorrow or early Wednesday.

[sharmilajesupaul]

@peanutenthusiast instead of trying to find the parent and placing the comment before it. What if you got the line number from the node range and placed the comment on the line before that with a prepended \n?

Also, does this account for cases where you are in an object, or a method chain, or array?

eg:

const foo = {
  id: 'bar'
  prop: (foo1) =>
    // comment
    'returning this string', 
}

[ foo => 
  // comment
  'bar'
]

"foo".split('').map((char) => 
  // comment
  `-${char}-`
))

can you add test cases for those? ^

[ghost]

@sharmilajesupaul will need to account for the aforementioned test cases as well. thanks!

[ghost]

@sharmilajesupaul Would the expected output for the method chain test case be

// comment
"foo".split('').map((char) => `-${char}-`
)

or (probably nicer on the eyes)

// comment
"foo".split('').map((char) => `-${char}-`)

?

[sharmilajesupaul]

@peanutenthusiast The paren linebreak is handled by function-paren-newline so I don't think you need to worry about including that in your autofix.

There might be cases where you do want the paren on it's own line:

"foo".split('').map(
  (char) => 
    `-${char}-`,
)

I think that might be out of the scope of this PR.

[ljharb]

this seems like it needs a fresh rebase

[ghost]

@ljharb agreed and working on it.

[sharmilajesupaul]

hey @peanutenthusiast! is this ready for another review?

[ghost]

Yes!

…

On Tue, Dec 4, 2018 at 5:13 PM Sharmila Jesupaul ***@***.***> wrote: hey @peanutenthusiast <https://github.com/peanutenthusiast>! is this ready for another review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10545 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aa-ISTkZKKdXwI4v_Ay3_rbsHsKqe_e7ks5u1x2vgaJpZM4U9zaM> .

[platinumazure]

Small change request for readability, otherwise looking good!

[nzakas]

@peanutenthusiast Sorry we lost track of this!

@platinumazure have your changes been addressed?

@ilyavolodin @not-an-aardvark thoughts on moving this forward?

[platinumazure]

Looks good to me, I'm okay to merge when the team accepts.

[not-an-aardvark]

Should this be marked as a bug rather than an enhancement? Fixers generally shouldn't be deleting comments.

[platinumazure]

This PR is labeled as "enhancement" but the commit summary says "Fix". Is this a bug fix or an enhancement?

[not-an-aardvark]

I'm going to re-label it as a bug -- I think the original labelling was a mistake.

[platinumazure]

Thanks for contributing @peanutenthusiast and for your saintly patience as we processed this!