This is very hard to define correctly as the type keyword sits between already supported tokens from the base rule that have their own defined before/after.
For now, this is how I think the defining cases should be but I'm open to changing them or add more as you see fit

I'm not sure if I should add support for an overrides option for type, and how to do it with extending the base rule

Omri Luzon added2 commits

November 26, 2022 07:52

fix/issue6063-keyword-spacing-unexpected-space-before

78f3749

remove some redundancy

91c18ff

Copy link

nx-cloudbot commentedNov 26, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commite8d5efc. 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 💌 fromNxCloud.

Copy link

Contributor

typescript-eslintbot commentedNov 26, 2022

Thanks for the PR,@omril1!

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 commentedNov 26, 2022•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`e8d5efc`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6388fa61a7e44c000986d84d
😎 Deploy Preview	https://deploy-preview-6095--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.

omril1 changed the title~~fix(eslint-plugin)keyword-spacing unexpected space before/after in import type~~fix(eslint-plugin):keyword-spacing unexpected space before/after in import type

Nov 26, 2022

omril1 changed the title~~fix(eslint-plugin):keyword-spacing unexpected space before/after in import type~~fix(eslint-plugin): [keyword-spacing] unexpected space before/after in import type

Nov 26, 2022

Copy link

codecovbot commentedNov 26, 2022•
edited
Loading

Codecov Report

Merging#6095 (e8d5efc) intomain (1dba257) willdecrease coverage by0.00%.
The diff coverage is91.66%.

Additional details and impacted files

@@            Coverage Diff             @@##             main    #6095      +/-   ##==========================================- Coverage   91.27%   91.26%   -0.01%==========================================  Files         366      366                Lines       12419    12427       +8       Branches     3632     3638       +6     ==========================================+ Hits        11335    11342       +7  Misses        774      774- Partials      310      311       +1

Flag	Coverage Δ
unittest	`91.26% <91.66%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...ackages/eslint-plugin/src/rules/keyword-spacing.ts	`96.77% <91.66%> (-3.23%)`	⬇️

JoshuaKGoldberg reviewed

Nov 26, 2022

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.

Did you mean to put this as a draft? Since it doesn't yet address the original issue, I'll convert it. Let me know if anything's unclear. And thanks for working on this!

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

		node.specifiers?.[0]?.type===AST_NODE_TYPES.ImportDefaultSpecifier
		){
		return;
		}

Copy link

Member

JoshuaKGoldbergNov 26, 2022

Choose a reason for hiding this comment

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

I'm confused. The preview deploy's playground still shows an unexpected space for one of the three issue's sample cases:https://deploy-preview-6095--typescript-eslint.netlify.app/play/#ts=4.9.3&sourceType=module&code=FASwtgDg9gTgLgAjgTwgUwQbwQZQIYBuaAJgIoCuaMIaAzggL4IBmMUYCARAHQD0+RMpWp1uAK1qcA3KEixEKdAgAWcOBABMLNh04A7KMTQAuVeo3TZ0eElQYzEbey4GjptREtA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Aa0UIHcB7aACa1kxAIZlmAc3RgA2uGw5uo6E0gAaBYqyLskAEaIAZv0Qz8cRJt17RR-Imgyjo+MitbdkXgDdHVARQZHRscRDczDBDQnDsHJwwLWERPGwBfaxioaER8WFVg1NDIOMdzSyLFDMq9C0JCrNt7MsSKxurGyDJRfDIACwbGqFKEsBcImogOrK7Rd0HOkfLkybBpmK7eJmR8BZml1pX2zI38Puhebj2Ng7AklOPVyCRdqNXY5tH71fXigV5rsVbt9Hp1eEZATYSp9lg8sr8oXNIV5gW14SdikZmK54PU3kNhjDDnCYgivEYLgBbZGKaHxWE-DFQyiU4j8V6Yd6E+nExlPRAADzZ0A50RuRLuaNJTK8gRcCFFXLpLUlR3hNTSRU1igAugpNWkgA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA. Am I missing something?

importtype{SavedQueries}from"./SavedQueries.js";

Copy link

ContributorAuthor

omril1Nov 26, 2022

Choose a reason for hiding this comment

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

I thought draft means you won't review it (Sorry, I guess it meant something else to me).

I opened it for review because I can't think of a way to solve it without adding an override option for thetype keyword or should it use the existing option for theimport keyword.

Copy link

Member

JoshuaKGoldbergNov 26, 2022

Choose a reason for hiding this comment

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

Oh I just didn't realize there was a question associated with this! Sorry, i misinterpreted the discussion. Will think on it 🙂

Copy link

Member

JoshuaKGoldbergNov 26, 2022

Choose a reason for hiding this comment

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

Or, actually, definitely yes. Since thetype option doesn't exist in ESLint core and is TypeScript-specific, we should add in a feature for it. Yes please 😄

Copy link

ContributorAuthor

omril1Nov 26, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Any example I can look at how to do that? The types seem looser than the schema validation (typed asoverrides?: Record<string, { before?: boolean; after?: boolean;}> but addingtype: { after: true } to object breaks the schema validation

OK, I found something similar to theno-magic-numbers rule

JoshuaKGoldberg marked this pull request as draft

November 26, 2022 20:37

Omri Luzon added5 commits

November 26, 2022 23:30

Merge remote-tracking branch 'upstream/main' into fix/issue6063-keywo…

4d58a46

…rd-spacing-unexpected-space-before

Add overrides for type token

ac5f9bd

Merge remote-tracking branch 'upstream/main' into fix/issue6063-keywo…

78a634c

…rd-spacing-unexpected-space-before

Merge remote-tracking branch 'upstream/main' into fix/issue6063-keywo…

e70260a

…rd-spacing-unexpected-space-before

test

e8d5efc

omril1 marked this pull request as ready for review

December 1, 2022 19:46

bradzacher approved these changes

Dec 2, 2022

View reviewed changes

Copy link

Member

bradzacher left a comment

Choose a reason for hiding this comment

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

thanks so much for following up to fix this!

bradzacher changed the title~~fix(eslint-plugin): [keyword-spacing] unexpected space before/after in import type~~fix(eslint-plugin): [keyword-spacing] unexpected space before/after inimport type

Dec 2, 2022

bradzacher merged commit98caa92 intotypescript-eslint:main

Dec 2, 2022

omril1 deleted the fix/issue6063-keyword-spacing-unexpected-space-before branch

December 2, 2022 04:42

github-actionsbot locked asresolvedand limited conversation to collaborators

Dec 10, 2022

Labels

None yet

Movatterモバイル変換

Uh oh!

fix(eslint-plugin): [keyword-spacing] unexpected space before/after inimport type#6095

fix(eslint-plugin): [keyword-spacing] unexpected space before/after inimport type#6095

Uh oh!

Conversation

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

Uh oh!

PR Checklist

Overview

Uh oh!

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

Uh oh!

☁️ Nx Cloud Report

Uh oh!

typescript-eslintbot commentedNov 26, 2022

Uh oh!

netlifybot commentedNov 26, 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 26, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergNov 26, 2022

Choose a reason for hiding this comment

Uh oh!

omril1Nov 26, 2022

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergNov 26, 2022

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergNov 26, 2022

Choose a reason for hiding this comment

Uh oh!

omril1Nov 26, 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!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(eslint-plugin): [keyword-spacing] unexpected space before/after in`import type`#6095

fix(eslint-plugin): [keyword-spacing] unexpected space before/after in`import type`#6095

omril1 commentedNov 26, 2022•
edited
Loading

nx-cloudbot commentedNov 26, 2022•
edited
Loading

netlifybot commentedNov 26, 2022•
edited
Loading

codecovbot commentedNov 26, 2022•
edited
Loading

omril1Nov 26, 2022•
edited
Loading