fix(eslint-plugin): [consistent-type-definitions] don't leave trailing parens when fixing type to interface #10235

kirkwaiblinger · 2024-10-30T03:41:18Z

PR Checklist

Addresses an existing open issue: fixes Bug: consistent-type-definitions leaves trailing parenthesis when converting a paren-wrapped type to an interface #10233
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

rewrite the fixer to be more robust

typescript-eslint · 2024-10-30T03:41:20Z

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.

netlify · 2024-10-30T03:41:33Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`c5d5163`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/672822ac6280720008ffa047
😎 Deploy Preview	https://deploy-preview-10235--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (🔴 down 1 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 your Netlify site configuration.

nx-cloud · 2024-10-30T03:44:29Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c5d5163. 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 2 targets

Sent with 💌 from NxCloud.

codecov · 2024-10-30T03:59:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.50%. Comparing base (4af866a) to head (c5d5163).
Report is 22 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10235   +/-   ##
=======================================
  Coverage   86.50%   86.50%           
=======================================
  Files         430      430           
  Lines       15088    15118   +30     
  Branches     4380     4396   +16     
=======================================
+ Hits        13052    13078   +26     
- Misses       1679     1683    +4     
  Partials      357      357

Flag	Coverage Δ
unittest	`86.50% <100.00%> (+<0.01%)`	⬆️

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

Files with missing lines	Coverage Δ
...nt-plugin/src/rules/consistent-type-definitions.ts	`100.00% <100.00%> (ø)`

... and 5 files with indirect coverage changes

auvred

Nice!

bradzacher · 2024-11-03T11:40:56Z

packages/eslint-plugin/src/rules/consistent-type-definitions.ts

+              // remove all closing parens, and the semicolon if present.
+              let rangeStart = node.typeAnnotation.range[1];
+              while (true) {
+                const afterToken =
+                  context.sourceCode.getTokenByRangeStart(rangeStart);
+
+                if (afterToken != null) {
+                  if (afterToken.value === ')') {
+                    fixes.push(fixer.remove(afterToken));
+                    rangeStart = afterToken.range[1];
+                    continue;
+                  } else if (
+                    afterToken.type === AST_TOKEN_TYPES.Punctuator &&
+                    afterToken.value === ';'
+                  ) {
+                    fixes.push(fixer.remove(afterToken));
+                    break;
+                  }
+                }
+
+                break;
              }


The better way to do this might be to do (psuedocode):

getTokenBefore(typeAnnotation, '=')

getTokenAfter(typeAnnotation, ';')

replace range from (1) to (2) with typEAnnotation

^Agreed; this feels a bit hacky

yeah, this was a good call. Result is much cleaner now 👍

bradzacher · 2024-11-03T23:40:47Z

packages/eslint-plugin/tests/rules/consistent-type-definitions.test.ts

+      code: noFormat`
+type Foo = ((((((((({
+  a: string;
+})))))))));
+      `,


Could you add a test case like

type Foo = ((({ a: string; }))) // next statement has a semi const bar = 1;

To ensure we are handling the no-semi case?

sure! added 👍

Note that the risk of messing up a semi/no-semi issue is also lessened now that the fixer isn't querying for it; it's just deleting up to node.range[1].

@auvred

##### [v8.13.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8130-2024-11-04) ##### 🚀 Features - **eslint-plugin:** \[only-throw-error] add allow option ([#10221](typescript-eslint/typescript-eslint#10221)) - **eslint-plugin:** \[prefer-nullish-coalescing] add option `ignoreBooleanCoercion` ([#9924](typescript-eslint/typescript-eslint#9924)) - **eslint-plugin:** disable `no-class-assign` rule in `eslint-recommended` config ([#10250](typescript-eslint/typescript-eslint#10250)) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] add support for covering a missing property with `undefined` ([#10232](typescript-eslint/typescript-eslint#10232)) - **eslint-plugin:** \[consistent-type-definitions] don't leave trailing parens when fixing type to interface ([#10235](typescript-eslint/typescript-eslint#10235)) - **eslint-plugin:** \[no-deprecated] report when exported class implements/extends deprecated entity ([#10259](typescript-eslint/typescript-eslint#10259)) - **eslint-plugin:** \[no-deprecated] report on deprecated variables used inside dynamic imports ([#10261](typescript-eslint/typescript-eslint#10261)) - **eslint-plugin:** \[no-unnecessary-condition] falsey bigint should be falsey ([#10205](typescript-eslint/typescript-eslint#10205)) ##### ❤️ Thank You - auvred [@auvred](https://github.com/auvred) - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Mark de Dios [@peanutenthusiast](https://github.com/peanutenthusiast) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@auvred

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.12.2 | 8.13.0 | | npm | @typescript-eslint/parser | 8.12.2 | 8.13.0 | ## [v8.13.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8130-2024-11-04) ##### 🚀 Features - **eslint-plugin:** \[only-throw-error] add allow option ([#10221](typescript-eslint/typescript-eslint#10221)) - **eslint-plugin:** \[prefer-nullish-coalescing] add option `ignoreBooleanCoercion` ([#9924](typescript-eslint/typescript-eslint#9924)) - **eslint-plugin:** disable `no-class-assign` rule in `eslint-recommended` config ([#10250](typescript-eslint/typescript-eslint#10250)) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] add support for covering a missing property with `undefined` ([#10232](typescript-eslint/typescript-eslint#10232)) - **eslint-plugin:** \[consistent-type-definitions] don't leave trailing parens when fixing type to interface ([#10235](typescript-eslint/typescript-eslint#10235)) - **eslint-plugin:** \[no-deprecated] report when exported class implements/extends deprecated entity ([#10259](typescript-eslint/typescript-eslint#10259)) - **eslint-plugin:** \[no-deprecated] report on deprecated variables used inside dynamic imports ([#10261](typescript-eslint/typescript-eslint#10261)) - **eslint-plugin:** \[no-unnecessary-condition] falsey bigint should be falsey ([#10205](typescript-eslint/typescript-eslint#10205)) ##### ❤️ Thank You - auvred [@auvred](https://github.com/auvred) - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Mark de Dios [@peanutenthusiast](https://github.com/peanutenthusiast) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@auvred

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.12.2 | 8.13.0 | | npm | @typescript-eslint/parser | 8.12.2 | 8.13.0 | ## [v8.13.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8130-2024-11-04) ##### 🚀 Features - **eslint-plugin:** \[only-throw-error] add allow option ([#10221](typescript-eslint/typescript-eslint#10221)) - **eslint-plugin:** \[prefer-nullish-coalescing] add option `ignoreBooleanCoercion` ([#9924](typescript-eslint/typescript-eslint#9924)) - **eslint-plugin:** disable `no-class-assign` rule in `eslint-recommended` config ([#10250](typescript-eslint/typescript-eslint#10250)) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] add support for covering a missing property with `undefined` ([#10232](typescript-eslint/typescript-eslint#10232)) - **eslint-plugin:** \[consistent-type-definitions] don't leave trailing parens when fixing type to interface ([#10235](typescript-eslint/typescript-eslint#10235)) - **eslint-plugin:** \[no-deprecated] report when exported class implements/extends deprecated entity ([#10259](typescript-eslint/typescript-eslint#10259)) - **eslint-plugin:** \[no-deprecated] report on deprecated variables used inside dynamic imports ([#10261](typescript-eslint/typescript-eslint#10261)) - **eslint-plugin:** \[no-unnecessary-condition] falsey bigint should be falsey ([#10205](typescript-eslint/typescript-eslint#10205)) ##### ❤️ Thank You - auvred [@auvred](https://github.com/auvred) - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Mark de Dios [@peanutenthusiast](https://github.com/peanutenthusiast) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

[consistent-type-definitions] remove closing parens around a type

3565da9

kirkwaiblinger force-pushed the consistent-type-definitions-closing-parens branch from fdf117d to 3565da9 Compare October 30, 2024 03:41

cov

18939a1

auvred previously approved these changes Oct 30, 2024

View reviewed changes

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

bradzacher previously approved these changes Nov 3, 2024

View reviewed changes

bradzacher reviewed Nov 3, 2024

View reviewed changes

better fixer logic

0afb8fe

kirkwaiblinger dismissed stale reviews from bradzacher and auvred via 0afb8fe November 3, 2024 23:19

kirkwaiblinger requested a review from bradzacher November 3, 2024 23:21

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Nov 3, 2024

kirkwaiblinger requested a review from auvred November 3, 2024 23:22

bradzacher reviewed Nov 3, 2024

View reviewed changes

test fixer edge cases

bf16cf2

kirkwaiblinger requested a review from bradzacher November 4, 2024 01:09

bradzacher previously approved these changes Nov 4, 2024

View reviewed changes

unused imports

c5d5163

kirkwaiblinger dismissed bradzacher’s stale review via c5d5163 November 4, 2024 01:26

bradzacher approved these changes Nov 4, 2024

View reviewed changes

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

bradzacher merged commit 5bd4c0a into typescript-eslint:main Nov 4, 2024
64 checks passed

kirkwaiblinger deleted the consistent-type-definitions-closing-parens branch November 4, 2024 06:15

github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [consistent-type-definitions] don't leave trailing parens when fixing type to interface #10235

fix(eslint-plugin): [consistent-type-definitions] don't leave trailing parens when fixing type to interface #10235

kirkwaiblinger commented Oct 30, 2024 •

edited

Loading

typescript-eslint bot commented Oct 30, 2024

netlify bot commented Oct 30, 2024 •

edited

Loading

nx-cloud bot commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading

auvred left a comment

bradzacher Nov 3, 2024

Josh-Cena Nov 3, 2024

kirkwaiblinger Nov 3, 2024

bradzacher Nov 3, 2024

kirkwaiblinger Nov 4, 2024

kirkwaiblinger Nov 4, 2024

fix(eslint-plugin): [consistent-type-definitions] don't leave trailing parens when fixing type to interface #10235

fix(eslint-plugin): [consistent-type-definitions] don't leave trailing parens when fixing type to interface #10235

Conversation

kirkwaiblinger commented Oct 30, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Oct 30, 2024

netlify bot commented Oct 30, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Oct 30, 2024 • edited Loading

☁️ Nx Cloud Report

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

auvred left a comment

Choose a reason for hiding this comment

bradzacher Nov 3, 2024

Choose a reason for hiding this comment

Josh-Cena Nov 3, 2024

Choose a reason for hiding this comment

kirkwaiblinger Nov 3, 2024

Choose a reason for hiding this comment

bradzacher Nov 3, 2024

Choose a reason for hiding this comment

kirkwaiblinger Nov 4, 2024

Choose a reason for hiding this comment

kirkwaiblinger Nov 4, 2024

Choose a reason for hiding this comment

kirkwaiblinger commented Oct 30, 2024 •

edited

Loading

netlify bot commented Oct 30, 2024 •

edited

Loading

nx-cloud bot commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading