Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Oct 25, 2019

Conversation

@scottohara
Copy link
Contributor

scottohara commented Sep 18, 2019

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

This comment has been minimized.

Copy link

jsf-clabot commented Sep 18, 2019

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Sep 18, 2019
@scottohara scottohara force-pushed the scottohara:function-call-argument-newline branch from e7909a4 to c8e013f Sep 18, 2019
@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Sep 18, 2019

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

This comment has been minimized.

Copy link

klimashkin commented Sep 27, 2019

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

@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Sep 27, 2019

It's reopened now

Copy link
Member

mdjermanovic left a comment

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

This comment has been minimized.

Copy link
Member

mdjermanovic commented Oct 18, 2019

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

@scottohara scottohara changed the title Fix: false positives on newlines in object/array arguments Update: fix function-call-argument-newline false positives on newlines in object/array arguments Oct 18, 2019
@scottohara scottohara changed the title Update: fix function-call-argument-newline false positives on newlines in object/array arguments Update: fix false positives in function-call-argument-newline (fixes #12123) Oct 18, 2019
@scottohara scottohara changed the title Update: fix false positives in function-call-argument-newline (fixes #12123) Update: false positives in function-call-argument-newline (fixes #12123) Oct 18, 2019
@scottohara

This comment has been minimized.

Copy link
Contributor Author

scottohara commented Oct 18, 2019

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.

@scottohara scottohara requested a review from mdjermanovic Oct 18, 2019
Copy link
Member

mdjermanovic left a comment

LGTM, thanks!

Copy link
Member

platinumazure left a comment

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

Copy link
Member

btmills left a comment

LGTM, thanks!

@btmills btmills merged commit 39dfe08 into eslint:master Oct 25, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191018.1 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@scottohara scottohara deleted the scottohara:function-call-argument-newline branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.