fix(eslint-plugin): [prefer-nullish-coalescing] mal-fixes nested conditional expression #11098
Conversation
…y are missing in the original code
|
Thanks for the PR, @mdm317! 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 configuration. |
|
View your CI Pipeline Execution ↗ for commit 07a6d71.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11098 +/- ##
=======================================
Coverage 90.83% 90.84%
=======================================
Files 497 497
Lines 50259 50273 +14
Branches 8291 8294 +3
=======================================
+ Hits 45652 45668 +16
+ Misses 4592 4590 -2
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Looks good! |
| @@ -932,3 +934,17 @@ function formatComments( | |||
| ) | |||
| .join(''); | |||
| } | |||
|
|
|||
| function getTextWithEnsuredParentheses( | |||
There was a problem hiding this comment.
I wonder if we can leverage existing infrastructure around parenthesization/wrapping rather than hard-code a special case for a ? b : c... is there a specific reason to do that? I worry we might miss parens on other expressions that might need it?
FWIW I had success passing all the test cases with the following...
context.report({
node,
messageId: 'preferNullishOverTernary',
// TODO: also account for = in the ternary clause
data: { equals: '' },
suggest: [
{
messageId: 'suggestNullish',
data: { equals: '' },
fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix {
return fixer.replaceText(
node,
`${getTextWithParentheses(
context.sourceCode,
nullishCoalescingParams.nullishCoalescingLeftNode,
)} ?? ${(() => {
const branch = getBranchNodes(
node,
operator,
).nullishBranch;
if (isParenthesized(branch, context.sourceCode)) {
return getTextWithParentheses(
context.sourceCode,
branch,
);
}
return getWrappedCode(
getTextWithParentheses(context.sourceCode, branch),
getOperatorPrecedenceForNode(branch),
OperatorPrecedence.Coalesce,
);
})()}`,
);
},
},
],
});Curious what you think!
There was a problem hiding this comment.
It definitely seems much better to use the existing infrastructure rather than hard-coding it!
I've updated the code to use your approach.
Thanks for pointing it out!
PR Checklist
Overview
When running the
Fix, add parentheses to conditional expressions that are missing them