-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
feat: fix indent rule for else-if #17318
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.
I love the new test cases, would appreciate if we can add a few more invalid
cases matching the valid ones for completeness.
code: unIndent` | ||
if (foo) | ||
\tif (bar) doSomething(); | ||
\telse doSomething(); | ||
else | ||
if (bar) doSomething(); | ||
else doSomething(); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll add more invalid
test cases to verify that this works well.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. I'm kind of shocked indent works at all given the complexity. Leaving open to see if we want to add more test cases.
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! Leaving open while a patch release is pending.
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Awesome, thanks!
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. |
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
indent
rule 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
if
in 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.