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: no-cond-assign with always
option reports switch case clauses
#12470
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.
This change looks good to me.
That said, why don't we just change the documentation to reflect what the rule actually enforces? Or is the problem that switch cases are not really "conditions", in your view?
I'm sure this wasn't intended behavior, there are several reasons (not just the documentation). But, yes it could become intended :-) I think that the rule aims to prevent the mistake of using switch (true) {
case x === 1: // ...
case x === 2: // ...
} So it might make sense to report I could open an enhancement issue to evaluate should the rule support |
Actually, a problem is that the change to start reporting We would also have to modify the Instead of all that, we could just document the current behavior, which is that the |
You've persuaded me. I think your change is the least worst approach here. Thanks! |
I agree, this is most likely the least confusing solution. Removing the Do Not Merge label then. |
I agree that it makes sense to make the current behavior consistent and document it before making any enhancements. |
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)
[X] Bug fix
This bug fix can produce only fewer warnings.
Tell us about your environment
What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue.
Online Demo Link
What did you expect to happen?
No errors. Per the documentation, this rule doesn't target switch case clauses.
What actually happened? Please include the actual, raw output from ESLint.
What changes did you make? (Give an overview)
Check parent statement's type.
Is there anything you'd like reviewers to focus on?
This test case was already valid:
These test cases were invalid, that's the bug fixed by this PR:
This test case was added for regression, as there were no test cases for
always
withConditionalExpression
. It was already invalid:I'll open a separate issue for conditional expressions.