fix(eslint-plugin): [no-unnecessary-boolean-literal-compare] fixer should handle parentheses #6569
Conversation
…ould handle parentheses
|
Thanks for the PR, @armano2! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
…olean-literal-compare-braces
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6569 +/- ##
==========================================
+ Coverage 90.66% 90.71% +0.04%
==========================================
Files 376 376
Lines 12851 12855 +4
Branches 3783 3783
==========================================
+ Hits 11651 11661 +10
+ Misses 856 850 -6
Partials 344 344
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| yield fixer.replaceText( | ||
| node, | ||
| sourceCode.getText(comparison.expression), | ||
| ); | ||
|
|
||
| // if the expression `exp` isn't nullable, or we're comparing to `true`, | ||
| // we can just replace the entire comparison with `exp` or `!exp` |
There was a problem hiding this comment.
after further investigation we could simplify this fixer to
fix: function* (fixer) {
const isUnaryNegation =
node.parent != null && nodeIsUnaryNegation(node.parent);
const mutatedNode = isUnaryNegation ? node.parent! : node;
yield fixer.replaceText(
mutatedNode,
sourceCode.getText(comparison.expression),
);
const shouldNegate = comparison.literalBooleanInComparison
? !comparison.negated
: comparison.negated;
if (isUnaryNegation === shouldNegate) {
yield fixer.insertTextBefore(mutatedNode, '!');
if (!util.isStrongPrecedenceNode(comparison.expression)) {
yield fixer.insertTextBefore(mutatedNode, '(');
yield fixer.insertTextAfter(mutatedNode, ')');
}
}
// if the expression `exp` is nullable, and we're not comparing to `true`,
// we can just replace the entire comparison with `exp` or `!exp`
if (
comparison.expressionIsNullableBoolean &&
!comparison.literalBooleanInComparison
) {
// provide the default `true`
yield fixer.insertTextBefore(mutatedNode, '(');
yield fixer.insertTextAfter(mutatedNode, ' ?? true)');
}
}that should allow us to provide better fixer for
// source
if (!(varBoolean === false)) {}
// current
if (!(!varBoolean)) {}
// new
if (varBoolean) {}i'm not sure if we should push this with this fix
There was a problem hiding this comment.
I like it 😄 +1 to including it here. Also a +1 to filing a followup if you want to get this in sooner.
There was a problem hiding this comment.
I'm going to land this as-is for now, just so we can get this fix out with the next release.
We can follow-up next week with the changes to simplify this fixer later
| yield fixer.replaceText( | ||
| node, | ||
| sourceCode.getText(comparison.expression), | ||
| ); | ||
|
|
||
| // if the expression `exp` isn't nullable, or we're comparing to `true`, | ||
| // we can just replace the entire comparison with `exp` or `!exp` |
There was a problem hiding this comment.
I like it 😄 +1 to including it here. Also a +1 to filing a followup if you want to get this in sooner.
JoshuaKGoldberg
added
the
1 approval
bradzacher
added
awaiting response
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
JoshuaKGoldberg
deleted the
fix/no-unnecessary-boolean-literal-compare-braces
branch
PR Checklist
instanceof#6567Overview
Add missing handling for removal and adding parenthesis in rule fixer
new cases covered:
old fixer result
new fixer result
initially i was planning to extend range of fixer to include
)(but after further investigation this started generating to many edge caseseg.
(false) === x instanceof Error