typescript-eslint/typescript-eslintPublic

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

feat(eslint-plugin): [naming-convention] add support for default and namespace imports#7269

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 2 commits intotypescript-eslint:mainfrombinoche9:import-naming-convention

Oct 17, 2023

Merged

feat(eslint-plugin): [naming-convention] add support for default and namespace imports#7269

JoshuaKGoldberg merged 2 commits intotypescript-eslint:mainfrombinoche9:import-naming-convention

Oct 17, 2023

Conversation

Copy link

Contributor

binoche9 commentedJul 19, 2023

PR Checklist

Addresses an existing open issue:fixes[naming-convention] Add the ability to lint imports #2106
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Currently thenaming-convention rule cannot be used to enforce conventions on default and namespace imports. This PR introduces that capability by introducing animport selector anddefault andnamespace modifiers.

Named imports are not checked since they have to match the name from the imported file.

Copy link

Contributor

typescript-eslintbot commentedJul 19, 2023

Thanks for the PR,@binoche9!

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 commentedJul 19, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`bc92533`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64d19e5a6f988700073ad170
😎 Deploy Preview	https://deploy-preview-7269--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 configuration.

Copy link

nx-cloudbot commentedJul 19, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitbc92533. 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 37 targets

Sent with 💌 fromNxCloud.

binoche9 commented

Jul 19, 2023

View reviewed changes

Copy link

ContributorAuthor

binoche9 left a comment

Choose a reason for hiding this comment

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

Let me know if there's any other place I should be making changes.

I'm also not sure if this counts as a breaking change since for people who have a default style it may break for them since imports that previously weren't matched will start getting the default style enforced.

packages/eslint-plugin/src/rules/naming-convention-utils/enums.ts

Comment on lines +47 to +48

		// other
		import = 1 << 18,

Copy link

ContributorAuthor

binoche9Jul 19, 2023

Choose a reason for hiding this comment

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

I decided it didn't belong to any selector group (I guessvariableLike comes closest) but feel free to disagree

packages/eslint-plugin/src/rules/naming-convention.ts

		modifiers.add(Modifiers.namespace);
		break;
		case AST_NODE_TYPES.ImportSpecifier:
		// Handle `import { default as Foo }`

Copy link

ContributorAuthor

binoche9Jul 19, 2023

Choose a reason for hiding this comment

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

I decidedimport { default as Foo } counts as a default import, but feel free to disagree

binoche9 changed the title~~feat(eslint-plugin) [naming-convention] add support for default and namespace imports~~feat(eslint-plugin): [naming-convention] add support for default and namespace imports

Jul 19, 2023

bradzacher added the enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule label

Jul 19, 2023

JoshuaKGoldberg previously approved these changes

Aug 5, 2023

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.

Generally looks great to me, thanks for sending in! 🙌

Just requesting changes on splitting the tests up. It all strikes me as reasonable otherwise though.

cc@bradzacher / will defer to you since you'd interacted with the feature request in#2106.

packages/eslint-plugin/tests/rules/naming-convention/naming-convention.test.tsShow resolvedHide resolved

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

Aug 5, 2023

binoche9 added2 commits

August 7, 2023 21:32

[naming-convention] add support for default and namespace imports

5d28d26

split tests

bc92533

binoche9 dismissedJoshuaKGoldberg’sstale review viabc92533

August 8, 2023 01:46

binoche9 force-pushed theimport-naming-convention branch from2e8df6e tobc92533Compare

August 8, 2023 01:46

Copy link

codecovbot commentedAug 8, 2023•
edited
Loading

Codecov Report

Merging#7269 (bc92533) intomain (0a1de14) willincrease coverage by0.01%.
The diff coverage is100.00%.

Additional details and impacted files

@@            Coverage Diff             @@##             main    #7269      +/-   ##==========================================+ Coverage   87.41%   87.42%   +0.01%==========================================  Files         381      381                Lines       13321    13332      +11       Branches     3938     3940       +2     ==========================================+ Hits        11645    11656      +11  Misses       1298     1298                Partials      378      378

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

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

Files Changed	Coverage Δ
...-plugin/src/rules/naming-convention-utils/enums.ts	`100.00% <ø> (ø)`
...plugin/src/rules/naming-convention-utils/schema.ts	`100.00% <ø> (ø)`
...kages/eslint-plugin/src/rules/naming-convention.ts	`90.68% <100.00%> (+0.53%)`	⬆️

Copy link

ContributorAuthor

binoche9 commentedAug 8, 2023

I split up the tests as requested 👍

Copy link

ContributorAuthor

binoche9 commentedAug 24, 2023

Friendly bump here!

JoshuaKGoldberg approved these changes

Oct 17, 2023

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.

Sorry for the delay! I just re-reviewed and it looks great. Thanks again for sending!

JoshuaKGoldberg merged commitbb15aae intotypescript-eslint:main

Oct 17, 2023

binoche9 deleted the import-naming-convention branch

October 18, 2023 14:51

Copy link

karlhorky commentedOct 24, 2023•
edited
Loading

Just upgraded to@typescript-eslint/eslint-plugin@6.9.0 and received a number of new warnings, such as this one:

checkbox/index.jsWarning:   1:8  warning  Import name `React` must match one of the following formats: camelCase  @typescript-eslint/naming-convention

With this code:

importReactfrom'react';

This is usingthe default rule configuration mentioned in the docs:

// the default config is similar to ESLint's camelcase rule but more strictconstdefaultOptions:Options=[{selector:'default',format:['camelCase'],leadingUnderscore:'allow',trailingUnderscore:'allow',},{selector:'variable',format:['camelCase','UPPER_CASE'],leadingUnderscore:'allow',trailingUnderscore:'allow',},{selector:'typeLike',format:['PascalCase'],},];

I guess this is going to be a pretty common pattern that gets rejected now. 😬

Maybe the default configuration should accept every format as an import? (format: null)

Alternative: expect all users with defaultPascalCase imports (pretty common across the npm ecosystem, incl. React) to add the following to their config:

// the default config is similar to ESLint's camelcase rule but more strictconst defaultOptions: Options = [  // ...+  // Disable @typescript-eslint/naming-convention format for imports+  // https://github.com/typescript-eslint/typescript-eslint/pull/7269#issuecomment-1777628591+  {+    selector: 'import',+    format: null,+  },];