Update: false positives in function-call-argument-newline (fixes #12123)

[scottohara]

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:

What changes did you make? (Give an overview)

Fixes #12123

Given two adjacent arguments (a & b), the rule logic was previously:

1. let prevArgToken be the first token of a
2. let currentArgToken be the first token of b
3. compare the start line number of prevArgToken with the start line number of currentArgToken

This meant that the following examples were treated as invalid, despite the linebreaks between arguments being correct for the specified option:

/*eslint function-call-argument-newline:["error", "never"] */
fn({
 a: 1
}, b);

/*eslint function-call-argument-newline:["error", "consistent"] */
fn({
 a: 1
}, b, c);

Changed the rule logic to be:

1. let prevArgToken be the last token of a
2. let currentArgToken be the first token of b
3. compare the end line number of prevArgToken with the start line number of currentArgToken

Valid & invalid tests added for each of the three options (always, never, consistent).

All new & all existing unit tests are passing.

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

Nothing in particular

[jsf-clabot]

All committers have signed the CLA.

[mdjermanovic]

Hi @scottohara, thanks for the PR!

Just to note that #12123 isn't accepted as a bug yet. It could be the correct behavior as well, or the rule might need an additional option to further customize the behavior in these cases with multiline arguments.

The proposed solution seems logical and I believe that it solves the problem from #12123. But, it can produce more warnings in some other cases, so I'm labeling this as an enhancement for now since it has to be evaluated.

[klimashkin]

@mdjermanovic What is the evaluation process? #12123 has been closed by the bot

[mdjermanovic]

It's reopened now

[mdjermanovic]

Changes in the rule LGTM and I think that the tests cover the getLastToken changes well.

Could we please also add some tests for the three loc.end changes? For example, this is an error at the moment, before this fix:

/* eslint function-call-argument-newline: ["error", "never"] */

f(`
`, a);

Online Demo Link

Also, the PR title should start with Update, because this change can produce more errors in some cases.

[mdjermanovic]

I forgot this - the PR title should also contain the name of this rule

[scottohara]

Thanks for the review @mdjermanovic.

I've added additional tests for the multi-line template string case as requested, and updated the PR title

With a long rule name like function-call-argument-newline, it was hard to get the title under 72-chars while still including both the Update: prefix and a (fixes #xxx) issue reference. Hopefully the current title is descriptive enough.

[mdjermanovic]

LGTM, thanks!

[platinumazure]

Looks good, thanks for contributing! Sorry for the delay in getting this in.

[btmills]

LGTM, thanks!