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
Update: false positives in function-call-argument-newline (fixes #12123) #12280
Conversation
e7909a4
to
c8e013f
Compare
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. |
@mdjermanovic What is the evaluation process? #12123 has been closed by the bot |
It's reopened now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
Also, the PR title should start with Update, because this change can produce more errors in some cases.
I forgot this - the PR title should also contain the name of this rule |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for contributing! Sorry for the delay in getting this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
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:prevArgToken
be the first token ofa
currentArgToken
be the first token ofb
prevArgToken
with the start line number ofcurrentArgToken
This meant that the following examples were treated as invalid, despite the linebreaks between arguments being correct for the specified option:
Changed the rule logic to be:
prevArgToken
be the last token ofa
currentArgToken
be the first token ofb
prevArgToken
with the start line number ofcurrentArgToken
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