typescript-eslint/typescript-eslintPublic

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

fix(eslint-plugin): [array-type] --fix flag removes parentheses from type#5997

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

bradzacher merged 1 commit intotypescript-eslint:mainfrommhnaeem:array-type-removes-parens

Nov 23, 2022

Merged

fix(eslint-plugin): [array-type] --fix flag removes parentheses from type#5997

bradzacher merged 1 commit intotypescript-eslint:mainfrommhnaeem:array-type-removes-parens

Nov 23, 2022

Conversation

Copy link

Contributor

mhnaeem commentedNov 16, 2022

PR Checklist

Addresses an existing open issue:fixesBug: [array-type]--fix removes neccesary brackets leading to a type with a different meaning #5941
That issue was marked asaccepting prs
Steps inCONTRIBUTING.md were taken

Overview

Array type with intersection that has parentheses does not retain the parens. Remove an unnecessary check since the type node doesn't include parens.

fix(eslint-plugin) [array-type] --fix flag removes parentheses from type

2fe9b96

Array type with intersection that has parentheses does not retain the parens.typescript-eslint#5941

Copy link

Contributor

typescript-eslintbot commentedNov 16, 2022

Thanks for the PR,@mhnaeem!

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

nx-cloudbot commentedNov 16, 2022•
edited
Loading

☁️ Nx Cloud Report

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

📂 See all runs for this branch

✅ Successfully ran 46 targets

Sent with 💌 fromNxCloud.

Copy link

netlifybot commentedNov 16, 2022•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`2fe9b96`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6374760ffd2f40000802d890
😎 Deploy Preview	https://deploy-preview-5997--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

Copy link

codecovbot commentedNov 16, 2022•
edited
Loading

Codecov Report

Merging#5997 (2fe9b96) intomain (2778ff0) willnot change coverage.
The diff coverage is100.00%.

Additional details and impacted files

@@           Coverage Diff           @@##             main    #5997   +/-   ##=======================================  Coverage   91.42%   91.42%           =======================================  Files         365      365             Lines       12339    12339             Branches     3609     3608    -1     =======================================  Hits        11281    11281             Misses        753      753             Partials      305      305

Flag	Coverage Δ
unittest	`91.42% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/array-type.ts	`97.14% <100.00%> (ø)`

JoshuaKGoldberg assignedJosh-Cena

Nov 16, 2022

Josh-Cena reviewed

Nov 16, 2022

View reviewed changes

Copy link

Member

Josh-Cena left a comment

Choose a reason for hiding this comment

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

Interesting—the fixer looks a bit hacky to me overall, but if there are no regressions, I like how simple the fix is.

packages/eslint-plugin/tests/rules/array-type.test.ts

Comment on lines +2010 to +2034

		testOutput(
		'array',
		`
		type WorkingArray = {
		outerProperty: Array<
		{ innerPropertyOne: string } & { innerPropertyTwo: string }
		>;
		}

		type BrokenArray = {
		outerProperty: Array<
		({ innerPropertyOne: string } & { innerPropertyTwo: string })
		>;
		}
		`,
		`
		type WorkingArray = {
		outerProperty: ({ innerPropertyOne: string } & { innerPropertyTwo: string })[];
		}

		type BrokenArray = {
		outerProperty: ({ innerPropertyOne: string } & { innerPropertyTwo: string })[];
		}
		`,
		);

Copy link

Member

Josh-CenaNov 16, 2022

Choose a reason for hiding this comment

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

This test case looks identical to the one above?

Copy link

ContributorAuthor

mhnaeemNov 16, 2022•
edited
Loading

Choose a reason for hiding this comment

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

@Josh-Cena One is with type and the other one is with interface, I wanted to be sure it doesn't affect the code different on either keyword. I can modify it a bit if you like@Josh-Cena

Maybe something like
type BrokenArray = Array<({ a: number } & { b: string })>

Copy link

ContributorAuthor

mhnaeem commentedNov 16, 2022

Interesting—the fixer looks a bit hacky to me overall, but if there are no regressions, I like how simple the fix is.

@Josh-Cena I could not think of any regressions and the tests passed as well, if you have any other test cases you would like me to include I will be happy to.

bradzacher added the bugSomething isn't working label

Nov 16, 2022

mhnaeem requested a review fromJosh-Cena

November 19, 2022 23:31

Josh-Cena approved these changes

Nov 19, 2022

View reviewed changes

Copy link

Member

Josh-Cena left a comment

Choose a reason for hiding this comment

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

Thanks, it seems to work 👍

Copy link

Member

Josh-Cena commentedNov 19, 2022

For the record, the code was changed in#3340

bradzacher approved these changes

Nov 23, 2022

View reviewed changes

bradzacher merged commit5e2f852 intotypescript-eslint:main

Nov 23, 2022

bradzacher pushed a commit that referenced this pull request

Nov 23, 2022

fix(eslint-plugin): [array-type] --fix flag removes parentheses from …

42b33af

…type (#5997)

bradzacher changed the title~~fix(eslint-plugin) [array-type] --fix flag removes parentheses from type~~fix(eslint-plugin): [array-type] --fix flag removes parentheses from type

Nov 23, 2022

github-actionsbot locked asresolvedand limited conversation to collaborators

Dec 1, 2022

Labels

bug

Something isn't working

Movatterモバイル変換

Uh oh!

fix(eslint-plugin): [array-type] --fix flag removes parentheses from type#5997

fix(eslint-plugin): [array-type] --fix flag removes parentheses from type#5997

Uh oh!

Conversation

mhnaeem commentedNov 16, 2022

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedNov 16, 2022

Uh oh!

nx-cloudbot commentedNov 16, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

netlifybot commentedNov 16, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

codecovbot commentedNov 16, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Josh-Cena left a comment

Choose a reason for hiding this comment

Uh oh!

Josh-CenaNov 16, 2022

Choose a reason for hiding this comment

Uh oh!

mhnaeemNov 16, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhnaeem commentedNov 16, 2022

Uh oh!

Josh-Cena left a comment

Choose a reason for hiding this comment

Uh oh!

Josh-Cena commentedNov 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nx-cloudbot commentedNov 16, 2022•
edited
Loading

netlifybot commentedNov 16, 2022•
edited
Loading

codecovbot commentedNov 16, 2022•
edited
Loading

mhnaeemNov 16, 2022•
edited
Loading