-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(eslint-plugin): [prefer-optional-chain] only look at left operand for requireNullish
#8559
fix(eslint-plugin): [prefer-optional-chain] only look at left operand for requireNullish
#8559
Conversation
Thanks for the PR, @abrahamguo! 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8559 +/- ##
==========================================
- Coverage 88.02% 87.24% -0.78%
==========================================
Files 405 252 -153
Lines 14089 12344 -1745
Branches 4125 3882 -243
==========================================
- Hits 12402 10770 -1632
+ Misses 1382 1305 -77
+ Partials 305 269 -36
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 haven't looked deep into the source code of the rule yet, so I can't suggest any complete solutions. Requesting changes to prevent one potential bug and to improve the tests.
It also looks like you linked the wrong issue in the PR description. I believe you should link this one: #8487. Right?
packages/eslint-plugin/src/rules/prefer-optional-chain-utils/gatherLogicalOperands.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/prefer-optional-chain-utils/gatherLogicalOperands.ts
Outdated
Show resolved
Hide resolved
…refer-optional-chain
…refer-optional-chain
…refer-optional-chain
…refer-optional-chain
packages/eslint-plugin/src/rules/prefer-optional-chain-utils/checkNullishAndReport.ts
Show resolved
Hide resolved
…refer-optional-chain
…refer-optional-chain
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.
The overall code changes look great to me! Although, I haven't gotten deep enough into the rule code to 100% understand the logic yet. I'll try to take the time to figure out how it works internally, and then I can do a more thorough review 🙂
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.
Mostly LGTM!
But there is conflict with main
branch in packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts
file. Could you please resolve it?
packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts
Outdated
Show resolved
Hide resolved
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 for working on this 🚀
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!
Since @bradzacher touched a ton of this stuff in #6397, will wait for a bit in case Brad has time to review.
PR Checklist
requireNullish
istrue
#8487Overview
As mentioned in the linked issue,
requireNullish: true
wrongly disables all checking for this rule. This is because it mistakenly applies therequireNullish
logic to both the left and right operands, rather than only the left.