fix(eslint-plugin): [prefer-find] support ternary branches in prefer-find #8421
Conversation
|
Thanks for the PR, @kirkwaiblinger! 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 #8421 +/- ##
==========================================
+ Coverage 86.94% 87.06% +0.11%
==========================================
Files 252 251 -1
Lines 12251 12278 +27
Branches 3861 3870 +9
==========================================
+ Hits 10652 10690 +38
+ Misses 1332 1316 -16
- Partials 267 272 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -584,5 +590,72 @@ arr.find(f, thisArg); | |||
| }, | |||
| ], | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
[Testing] Just to be through, could you also add a test for a desired report with and within a ternary? Like the valid case above, but either with two .filters or with the [0] on the other side of the last )?
(Math.random() < 0.5
? [1, 2, 3].filter(x => false)
: [1, 2, 3].filter(x => true))[0];(Math.random() < 0.5
? [1, 2, 3].find(x => true)
: [1, 2, 3].filter(x => true)[0]);The cases lower down are a bit more complex. It's nice to have more straightforward ones to use as a reference too.
| @@ -584,5 +590,72 @@ arr.find(f, thisArg); | |||
| }, | |||
| ], | |||
| }, | |||
| { | |||
| code: ` | |||
| declare const f: any, g: any; | |||
There was a problem hiding this comment.
[Testing] Nit: here and below, if there's no need to use an any, might as well avoid it:
| declare const f: any, g: any; | |
| declare const f: unknown, g: unknown; |
There was a problem hiding this comment.
it would have a type error without. I was just too lazy to write out the actual type of the filter callback (and the rule doesn't care about the type of the filter callback) so i went for the easiest way to not error :)
There was a problem hiding this comment.
Personally, I'm +1 to avoid semantic errors, at least in newly added tests :)
Especially when it's not that hard:
- declare const f: any, g: any;
+ declare const f: (x: unknown) => boolean, g: (x: unknown) => boolean;There was a problem hiding this comment.
Fair enough, changed
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
8ea5d4c to
9d1b79a
Compare
f397a3a
PR Checklist
Overview
Adds support for recursively checking ternary branches.