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
Fix: arrow-body-style fixer for in
wrap (fixes #11849)
#13228
Conversation
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.
Thanks for this work. 😀 I left some comments.
Co-authored-by: YeonJuan <yeonjuan93@naver.com>
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.
@anikethsaha
Sorry for the delay. 😂 I left comments about the code and unexpected behavior.
It seems to add extra parenthesis when it is already wrapped.
// input
for ( a = (b) => { return (c in d) }; ;);
// expected
for ( a = (b) => (c in d); ;);
// result of the auto fix - PR
for ( a = (b) => ((c in d)); ;);
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.
@anikethsaha
I tested this PR branch and the fixer seems to generate a syntax error code on these cases.
code: "for ( a = (b) => { return (1) + c in d }; ;);",
output: "for ( a = (b) => (1) + c in d; ;);",
I'm not sure what the proper result should be.
maybe wrapping all?
output: "for ( a = (b) => ((1) + c in d); ;);",
@yeonjuan thanks for pointing. I think , this check needs to be changed. I think there will be conflict to solve this 👇
and the one you mentioned in #13228 (review) ? What I am thinking is to track the opening braces and closing braces, if all opening braces are closed then the next token should be Not sure though. Edit:
or we can keep this ? |
Well, I'm not sure how we can treat it.
IMO, we should not keep it. |
@yeonjuan this is what I am trying to do by checking the leading What I can do it to check if they are with but then this case might be conflicting. for ( a = (b) => { return c in (d+c) }; ;); do you have any other way to check for wrapped or not.? maybe I am missing something here. |
@yeonjuan looks like there's a question for you on this PR |
There are many cases where those parens wouldn't be necessary: for (a = b => { return (c in d) } ;;) {}
for (a = b => { return [c in d] } ;;) {}
for (a = b => { return c || (d in e) } ;;) {}
for (a = b => { return c || (d && e in f) } ;;) {}
for (a = (b => { return c in d }) ;;) {}
for (a = [b => { return c in d }] ;;) {} A similar problem ( In my opinion, |
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.
So, regarding the question, I personally think we shouldn't care much about extra parens in this rule.
On the other hand, it seems that some cases where the parens are necessary are still not covered.
maybe we can do it by checking whether the whole returned node is wrapped by parenthesis (not only checking the first token of the node) -but not sure cause I didn't try it.
But, as the @mdjermanovic 's comment, it might be reasonable not to check extra parens in here strictly. Sorry if you got a confusion. |
@mdjermanovic I changed the code to a bit different, It seems working though, Let me know if this needs to be changed if so I will check with the approach you mentioned. |
It still doesn't work well in this case (we should probably add this as a test case): /*eslint arrow-body-style: ["error", "as-needed"]*/
a in b;
for (var f = () => { return c };;); auto-fixes to: /*eslint arrow-body-style: ["error", "as-needed"]*/
a in b;
for (var f = () => (c);;); This is the order of events for this example:
|
@mdjermanovic I did add a test case for the mentioned code here https://github.com/eslint/eslint/pull/13228/files#diff-a5a55cb049da483c38e374fd2af60fecR62 , is this what you were referring? not sure about
cool, I will check this
in this, are these two separate test cases or same ? |
1 test, something like: code: "a in b; for (var f = () => { return c };;);"
output: "a in b; for (var f = () => c;;);" (no parens in output) |
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!
@yeonjuan Have your concerns been addressed? |
@yeonjuan just checking in to see if you're ready to approve this latest version? |
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.
Sorry for the late, It LGTM!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an 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)
Fixed the fixer for
in
operator being wrongly fixed. wrapping the whole body when there is anin
operator.inIn
method will check if the body hasin
operator or not. wrapping the whole as mentioned in the issue was decided.Is there anything you'd like reviewers to focus on?
fixes #11849