chore: enable no-confusing-void-expression internally #8375
Conversation
|
Thanks for the PR, @JoshuaKGoldberg! 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8375 +/- ##
==========================================
+ Coverage 86.94% 87.79% +0.84%
==========================================
Files 252 398 +146
Lines 12251 13902 +1651
Branches 3861 4067 +206
==========================================
+ Hits 10652 12205 +1553
- Misses 1332 1400 +68
- Partials 267 297 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I'll wait to merge these changes until the flat config work lands. Edit: ✔️ |
There was a problem hiding this comment.
Looking at the changes - TBH I don't really like this rule.
I don't think any of these changes improve clarity or readability of code.
The biggest issue I see is that if the function is implicitly typed or explicitly marked as returning void the rule still requires you to change things.
For example this code should pass the rule, IMO:
function returnsVoid(): void {}
function test1(): void {
return returnsVoid(); // should be fine
}
const test2 = (): void => returnsVoid(); // should be fineBecause there's nothing confusing about that code - the expressions are all typed as void and the functions are typed as void.
Also changes like () => expr => () => { expr } actively reduce readability, IMO, esp when considering the above.
So I'm personally a bit 👎 on this.
|
👍 Blocked on #8538. |
PR Checklist
Overview
Enables the rule internally and runs its
--fixto correct all violations. The fixed style isn't always quite what I'd want, but I figured anything we definitively don't like should probably be enforced with separate lint rules:for..ofinstead of.forEach{}s around aswitchcaseunnecessarily