feat(eslint-plugin): add `key-spacing` rule extension for interface & type declarations #6211

coyotte508 · 2022-12-14T00:27:49Z

PR Checklist

Addresses an existing open issue: fixes [key-spacing] key-spacing of interfaces and other TS-specific key-value pairs #940
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Make key-spacing work with interfaces & type declarations. ~~🚧 Work in progress 🚧.~~

Happy to take any and all feedback & help. ~~Do I need to support all options & sub-options from the base rule? 😓~~ edit by @JoshuaKGoldberg: summarizing discussion, yes 😄

References

eslint rule: https://eslint.org/docs/latest/rules/key-spacing
eslint rule source: https://github.com/eslint/eslint/blob/main/lib/rules/key-spacing.js#L491

Todo

add more tests
add autofix
add doc

typescript-eslint · 2022-12-14T00:27:51Z

Thanks for the PR, @coyotte508!

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.

nx-cloud · 2022-12-14T00:27:51Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0184de9. 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 47 targets

Sent with 💌 from NxCloud.

netlify · 2022-12-14T00:27:52Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`0184de9`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/63d064362ba3200008ff3d39
😎 Deploy Preview	https://deploy-preview-6211--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 your Netlify site settings.

JoshuaKGoldberg · 2022-12-14T00:32:39Z

Do I need to support all options & sub-options from the base rule? 😓

Yup! 😁

coyotte508 · 2022-12-15T01:57:16Z

The base implementation is done - ~~I had to hack around a bit for index signatures: { [x: number]: string}, I found no way to get the position of the ] other than to check the text value of the whole node.~~

Key alignment is done for interface / types / classes properties & index signatures.

I still need to do the autofixer & the docs. And probably test more.

…tions

It defaults to 'colon' in this case

coyotte508 · 2023-01-24T17:12:43Z

Mainly requesting changes here for more thorough testing of edge cases and different options. Hopefully they don't make the code too much more lots-of-different-cases-y?

Just a few lines to handle unicode characters + a module that can be replaced by Intl.Segmenter when Node 14 stops being supported.

No if/else in added in the rule's logic :)

I hope that the painstaking annoying edge cases you're working through in this PR have at least helped emphasize why we strongly recommend nobody use key-spacing or other formatting rules`. smile

Can't say I disagree, if it was up to me I'd use prettier and call it a day 🤫

Still, this rule grows on you 🤷

I couldn't really address the MakeRequired comment, due to Typescript limitations I think. I marked the other comments as resolved, not sure it's the etiquette for this project or if the initial commenter should be the one to, feel free to reopen them in any case!

packages/eslint-plugin/src/rules/key-spacing.ts

JoshuaKGoldberg · 2023-01-24T21:49:33Z

not sure it's the etiquette for this project or if the initial commenter should be the one to

Tangent: I miss working in a review tool that differentiated between mark as resolved (code author's responsibility) and close (reviewer's responsibility). Alas. +1 to this being a not-clear thing on GitHub!

JoshuaKGoldberg

Looks great, thanks!

I do want to poke @bradzacher to make sure I'm not missing some dependency shenanigan with grapheme-splitter (same as #5704). But otherwise looks wonderful, thanks!

packages/eslint-plugin/src/rules/key-spacing.ts

JoshuaKGoldberg

bradzacher · 2023-01-31T13:28:22Z

packages/eslint-plugin/package.json

    "grapheme-splitter": "^1.0.4",
    "ignore": "^5.2.0",
+    "grapheme-splitter": "^1.0.4",


looks like a bad merge - added the entry twice!

99c091e#r98752909

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.48.2` -> `5.50.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.48.2/5.50.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.48.2` -> `5.50.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.48.2/5.50.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.50.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5500-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5490v5500-2023-01-31) [Compare Source](typescript-eslint/typescript-eslint@v5.49.0...v5.50.0) ##### Bug Fixes - **eslint-plugin:** \[ban-ts-comment] counts graphemes instead of `String.prototype.length` ([#5704](typescript-eslint/typescript-eslint#5704)) ([09d57ce](typescript-eslint/typescript-eslint@09d57ce)) - **eslint-plugin:** \[prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors ([#6028](typescript-eslint/typescript-eslint#6028)) ([85e783c](typescript-eslint/typescript-eslint@85e783c)) - **eslint-plugin:** \[prefer-optional-chain] fixer produces wrong logic ([#5919](typescript-eslint/typescript-eslint#5919)) ([b0f6c8e](typescript-eslint/typescript-eslint@b0f6c8e)), closes [#1438](typescript-eslint/typescript-eslint#1438) ##### Features - **eslint-plugin:** add `key-spacing` rule extension for interface & type declarations ([#6211](typescript-eslint/typescript-eslint#6211)) ([67706e7](typescript-eslint/typescript-eslint@67706e7)) ### [`v5.49.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5490-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5482v5490-2023-01-23) [Compare Source](typescript-eslint/typescript-eslint@v5.48.2...v5.49.0) ##### Features - **eslint-plugin:** \[naming-convention] add support for `#private` modifier on class members ([#6259](typescript-eslint/typescript-eslint#6259)) ([c8a6d80](typescript-eslint/typescript-eslint@c8a6d80)) #### [5.48.2](typescript-eslint/typescript-eslint@v5.48.1...v5.48.2) (2023-01-16) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) #### [5.48.1](typescript-eslint/typescript-eslint@v5.48.0...v5.48.1) (2023-01-09) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.50.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5500-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5490v5500-2023-01-31) [Compare Source](typescript-eslint/typescript-eslint@v5.49.0...v5.50.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) ### [`v5.49.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5490-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5482v5490-2023-01-23) [Compare Source](typescript-eslint/typescript-eslint@v5.48.2...v5.49.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.48.2](typescript-eslint/typescript-eslint@v5.48.1...v5.48.2) (2023-01-16) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.48.1](typescript-eslint/typescript-eslint@v5.48.0...v5.48.1) (2023-01-09) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1757 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

🚧 key-spacing for interface on default settings

eb02b4d

coyotte508 changed the title ~~feat(eslint-plugin): key-spacing for interface & type declarations~~ feat(eslint-plugin): support key-spacing for interface & type declarations Dec 14, 2022

coyotte508 added 7 commits December 14, 2022 01:51

🚧 Support type literals as welll

6d78fe8

🚧 Add full typing for the options

259a3ca

🚧 Add 'mode' param

8f32b5d

🐛 Fix index signatures

34bb6d8

✨ Support classes

ecaafa7

🩹 fixes

b17665b

✅ Add tests on mode, multiLine, singleLine

18bb8ab

coyotte508 added 4 commits December 15, 2022 03:15

🏷️ Allow options.multiline.align to be an object

4113478

🎨 Use ast utils to locate last character before token

5e951ba

✨ Support comments in-between properties

b12c530

✅ Add test cases for nested type declarations & multiline type annota…

729e642

…tions

coyotte508 force-pushed the key-spacing branch from 62a96ba to 729e642 Compare December 15, 2022 09:31

coyotte508 added 4 commits December 15, 2022 11:03

✨ Autofix for non-aligned values

d93c1a7

✨ Autofix for aligned values

ee519e3

✏️

bc3b5d2

🚨

eebb18f

coyotte508 force-pushed the key-spacing branch from 09b685c to eebb18f Compare December 15, 2022 10:38

🐛 Support optional ? token

179aca2

coyotte508 force-pushed the key-spacing branch from 69b27ef to 179aca2 Compare December 15, 2022 10:48

coyotte508 added 2 commits December 15, 2022 12:30

✅ Add tests with class property assignments

db2d1bd

📝 Add documentation on key-spacing

dbe502d

coyotte508 marked this pull request as ready for review December 15, 2022 11:51

🎨 Use .at() to access last element of array

8d5d3dd

coyotte508 added 3 commits January 24, 2023 15:32

✅ Add wacky test cases

b1033ef

✅ Add coverage

a7a5b2c

✅ Add coverage, again

cb5876e

coyotte508 force-pushed the key-spacing branch from 968ac14 to 54aed3b Compare January 24, 2023 16:14

✅ Add coverage, again

2787762

coyotte508 force-pushed the key-spacing branch from 54aed3b to 2787762 Compare January 24, 2023 16:15

✅ Add coverage when align is an object, but align.on is missing

79d8ec0

It defaults to 'colon' in this case

coyotte508 marked this pull request as ready for review January 24, 2023 17:13

coyotte508 requested a review from JoshuaKGoldberg January 24, 2023 17:13

coyotte508 commented Jan 24, 2023

View reviewed changes

packages/eslint-plugin/src/rules/key-spacing.ts Outdated Show resolved Hide resolved

KeyTypeNodeWithTypeAnnotation

2092487

JoshuaKGoldberg previously approved these changes Jan 24, 2023

View reviewed changes

JoshuaKGoldberg added 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting response Issues waiting for a reply from the OP or another party labels Jan 24, 2023

coyotte508 commented Jan 24, 2023

View reviewed changes

packages/eslint-plugin/src/rules/key-spacing.ts Show resolved Hide resolved

JoshuaKGoldberg added 3 commits January 24, 2023 18:02

Merge branch 'main' into key-spacing

23036de

Merge branch 'main' into key-spacing

e483407

Extract to shared helper

0184de9

JoshuaKGoldberg dismissed their stale review via 0184de9 January 24, 2023 23:05

JoshuaKGoldberg approved these changes Jan 24, 2023

View reviewed changes

JoshuaKGoldberg changed the title ~~feat(eslint-plugin): support key-spacing for interface & type declarations~~ feat(eslint-plugin): add key-spacing rule extension for interface & type declarations Jan 24, 2023

JoshuaKGoldberg merged commit 67706e7 into typescript-eslint:main Jan 24, 2023

bradzacher reviewed Jan 31, 2023

View reviewed changes

coyotte508 deleted the key-spacing branch January 31, 2023 13:35

JoshuaKGoldberg mentioned this pull request Jan 31, 2023

Rule proposal: [prefer-at] Rule to prefer array.at(-1) over array[array.length - 1] #6401

Open

6 tasks

aaneitchik mentioned this pull request Feb 1, 2023

feat: disable @typescript-eslint/key-spacing rule ridedott/eslint-config#1708

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): add `key-spacing` rule extension for interface & type declarations #6211

feat(eslint-plugin): add `key-spacing` rule extension for interface & type declarations #6211

coyotte508 commented Dec 14, 2022 •

edited by JoshuaKGoldberg

Loading

typescript-eslint bot commented Dec 14, 2022

nx-cloud bot commented Dec 14, 2022 •

edited

Loading

netlify bot commented Dec 14, 2022 •

edited

Loading

JoshuaKGoldberg commented Dec 14, 2022

coyotte508 commented Dec 15, 2022 •

edited

Loading

coyotte508 commented Jan 24, 2023

JoshuaKGoldberg commented Jan 24, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

bradzacher Jan 31, 2023

JoshuaKGoldberg Jan 31, 2023

JoshuaKGoldberg Jan 31, 2023

feat(eslint-plugin): add key-spacing rule extension for interface & type declarations #6211

feat(eslint-plugin): add key-spacing rule extension for interface & type declarations #6211

Conversation

coyotte508 commented Dec 14, 2022 • edited by JoshuaKGoldberg Loading

PR Checklist

Overview

References

Todo

typescript-eslint bot commented Dec 14, 2022

nx-cloud bot commented Dec 14, 2022 • edited Loading

☁️ Nx Cloud Report

netlify bot commented Dec 14, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

JoshuaKGoldberg commented Dec 14, 2022

coyotte508 commented Dec 15, 2022 • edited Loading

coyotte508 commented Jan 24, 2023

JoshuaKGoldberg commented Jan 24, 2023

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

bradzacher Jan 31, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 31, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 31, 2023

Choose a reason for hiding this comment

feat(eslint-plugin): add `key-spacing` rule extension for interface & type declarations #6211

feat(eslint-plugin): add `key-spacing` rule extension for interface & type declarations #6211

coyotte508 commented Dec 14, 2022 •

edited by JoshuaKGoldberg

Loading

nx-cloud bot commented Dec 14, 2022 •

edited

Loading

netlify bot commented Dec 14, 2022 •

edited

Loading

coyotte508 commented Dec 15, 2022 •

edited

Loading