typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

fix(eslint-plugin): [prefer-find] support ternary branches in prefer-find#8421

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JoshuaKGoldberg merged 7 commits intotypescript-eslint:mainfromkirkwaiblinger:add-support-for-ternaries

Mar 11, 2024

Merged

fix(eslint-plugin): [prefer-find] support ternary branches in prefer-find#8421

JoshuaKGoldberg merged 7 commits intotypescript-eslint:mainfromkirkwaiblinger:add-support-for-ternaries

Mar 11, 2024

Conversation

Copy link

Member

kirkwaiblinger commentedFeb 9, 2024

PR Checklist

Addresses an existing open issue:fixesBug: [prefer-find] Add support for checking ternaries #8379
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Adds support for recursively checking ternary branches.

8379: Support ternary branches in prefer-find

f372aba

Copy link

Contributor

typescript-eslintbot commentedFeb 9, 2024

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.

Copy link

netlifybot commentedFeb 9, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`9e599c0`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/65da2713759f0b0008172c9e
😎 Deploy Preview	https://deploy-preview-8421--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 90 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

Copy link

nx-cloudbot commentedFeb 9, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit9e599c0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 fromNxCloud.

kirkwaiblinger changed the title~~8379: Support ternary branches in prefer-find~~fix(eslint-plugin): [prefer-find] support ternary branches in prefer-find

Feb 9, 2024

Copy link

codecovbot commentedFeb 9, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.06%. Comparing base(9c94b81) to head(9e599c0).
Report is 63 commits behind head on main.

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

Flag	Coverage Δ
unittest	`87.06% <100.00%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Files	Coverage Δ
packages/eslint-plugin/src/rules/prefer-find.ts	`100.00% <100.00%> (ø)`

... and17 files with indirect coverage changes

kirkwaiblinger added2 commits

February 9, 2024 13:59

fix codecov

0db4183

lintfix

2200f83

kirkwaiblinger marked this pull request as ready for review

February 9, 2024 21:23

kirkwaiblinger closed this

Feb 21, 2024

kirkwaiblinger deleted the add-support-for-ternaries branch

February 21, 2024 21:07

kirkwaiblinger restored the add-support-for-ternaries branch

February 21, 2024 21:09

kirkwaiblinger reopened this

Feb 21, 2024

JoshuaKGoldberg reviewed

Feb 23, 2024

View reviewed changes

packages/eslint-plugin/src/rules/prefer-find.tsShow resolvedHide resolved

JoshuaKGoldberg requested changes

Feb 23, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Code changes look great, nicely done! 👏

Just requesting changes on a bit more testing.

packages/eslint-plugin/tests/rules/prefer-find.test.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/tests/rules/prefer-find.test.ts

		},
		],
		},
		{

Copy link

Member

JoshuaKGoldbergFeb 23, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[Testing] Just to be through, could you also add a test for a desired report with and within a ternary? Like thevalid 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.

packages/eslint-plugin/tests/rules/prefer-find.test.ts Outdated

		},
		{
		code:`
		declare const f: any, g: any;

Copy link

Member

JoshuaKGoldbergFeb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

[Testing] Nit: here and below, if there's no need to use anany, might as well avoid it:

Suggested change

	declareconstf:any,g:any;
	declareconstf:unknown,g:unknown;

Copy link

MemberAuthor

kirkwaiblingerFeb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 :)

Copy link

Member

JoshuaKGoldbergFeb 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ha, gotcha.#8298 ->#8351 tracks making tests error on type unhappiness. cc@auvred - do you think we should start asking folks to try to not have type errors?

Copy link

Member

auvredFeb 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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;

Copy link

MemberAuthor

kirkwaiblingerFeb 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fair enough, changed

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Feb 23, 2024

kirkwaiblingerand others added2 commits

February 23, 2024 08:58

Update packages/eslint-plugin/tests/rules/prefer-find.test.ts

7fd8285

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

add some straightforward tests

9d1b79a

kirkwaiblinger force-pushed theadd-support-for-ternaries branch from8ea5d4c to9d1b79aCompare

February 23, 2024 16:29

kirkwaiblinger requested a review fromJoshuaKGoldberg

February 24, 2024 04:12

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Feb 24, 2024

JoshuaKGoldberg previously approved these changes

Feb 24, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, nice! Since it's tricky logic being touched, will leave open for a bit under1 approval.

JoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Feb 24, 2024

auvred previously approved these changes

Feb 24, 2024

View reviewed changes

Copy link

Member

auvred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM 🚀

remove any

f397a3a

kirkwaiblinger dismissed stale reviews fromauvred andJoshuaKGoldberg viaf397a3a

February 24, 2024 17:18

tweak tests slightly

9e599c0

kirkwaiblinger requested review fromJoshuaKGoldberg andauvred

February 24, 2024 17:29

kirkwaiblinger commented

Feb 24, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/prefer-find.test.tsShow resolvedHide resolved

auvred approved these changes

Feb 24, 2024

View reviewed changes

Copy link

Member

auvred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM! Thanks for updating tests!

JoshuaKGoldberg merged commitc0e3267 intotypescript-eslint:main

Mar 11, 2024

kirkwaiblinger deleted the add-support-for-ternaries branch

March 11, 2024 13:31

github-actionsbot locked asresolvedand limited conversation to collaborators

Mar 19, 2024

Labels

1 approval

>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

Movatterモバイル変換

Uh oh!

fix(eslint-plugin): [prefer-find] support ternary branches in prefer-find#8421

fix(eslint-plugin): [prefer-find] support ternary branches in prefer-find#8421

Uh oh!

Conversation

kirkwaiblinger commentedFeb 9, 2024

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedFeb 9, 2024

Uh oh!

netlifybot commentedFeb 9, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

nx-cloudbot commentedFeb 9, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

codecovbot commentedFeb 9, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaKGoldbergFeb 23, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergFeb 24, 2024

Choose a reason for hiding this comment

Uh oh!

auvredFeb 24, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerFeb 24, 2024

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

auvred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

auvred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlifybot commentedFeb 9, 2024•
edited
Loading

nx-cloudbot commentedFeb 9, 2024•
edited
Loading

codecovbot commentedFeb 9, 2024•
edited
Loading

JoshuaKGoldbergFeb 23, 2024•
edited
Loading