feat: fix indent rule for else-if #17318
Conversation
mdjermanovic
added
bug
| code: unIndent` | ||
| if (foo) | ||
| \tif (bar) doSomething(); | ||
| \telse doSomething(); | ||
| else | ||
| if (bar) doSomething(); | ||
| else doSomething(); |
There was a problem hiding this comment.
Compared to all the great valid test cases, this single invalid case seems a bit... insufficient? 😅
Would you mind copying a few of them here (and make them invalid of course)?
There was a problem hiding this comment.
Good idea, I'll add more invalid test cases to verify that this works well.
There was a problem hiding this comment.
I've added more invalid tests in 9142eb2, please take a look.
| @@ -1250,7 +1250,7 @@ module.exports = { | |||
|
|
|||
| IfStatement(node) { | |||
| addBlocklessNodeIndent(node.consequent); | |||
| if (node.alternate && node.alternate.type !== "IfStatement") { | |||
There was a problem hiding this comment.
I was able to trace this logic all the way back to the big rewrite of this rule so I'm guessing it was carried over from broken logic earlier or was an inherent bug in the initial implementation.
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This fix has broken things, see : #17316 (comment) |
|
Yep, indent is broken for else lines. Using 8.44.0 for the time being as 8.45.0 reports plenty of indent errors in my case. |
|
Feel free to open an issue reporting this. Thanks. |
eslint-github-bot
bot
added
the
archived due to age
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 autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #17316
This bug fix can generate more warnings from the
indentrule in existing code, so marked asfeat.What changes did you make? (Give an overview)
Removed a condition that causes indentation not to be applied to
ifin else-if positions.Is there anything you'd like reviewers to focus on?
Please check if this change could cause any unwanted behavior. All existing tests are passing after this change, so I'm not sure which cases this condition was targeting. The condition was added in cc53481. I tried removing the same condition in that version, and some tests from that version are indeed failing, but those tests are still here and are now passing without the condition. I've added a few test cases that target this part of the code and the behavior seems fine to me.