-
-
Notifications
You must be signed in to change notification settings - Fork 57
feat: port react-x/prefer-react-namespace-import into prefer-namespace-import
#386
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 6e30b35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
WalkthroughA new ESLint rule, Changes
Sequence Diagram(s)sequenceDiagram
participant ESLint
participant Rule as prefer-namespace-import Rule
participant UserConfig
ESLint->>Rule: Analyze import declarations
Rule->>UserConfig: Retrieve patterns option
Rule->>Rule: Match import source against patterns
alt Default import matches pattern
Rule->>ESLint: Report error, suggest namespace import
alt Autofix enabled
Rule->>ESLint: Replace/split import with namespace import
end
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/index.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/rules/prefer-default-export.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/rules/prefer-namespace-import.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ports a new ESLint rule "prefer-namespace-import" which enforces using namespace imports for specific modules. Key changes include:
- Adding unit tests in test/rules/prefer-namespace-import.spec.ts
- Implementing the rule in src/rules/prefer-namespace-import.ts with an auto-fix feature
- Integrating the rule in the index, updating related exports, and documenting it in docs and README
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/rules/prefer-namespace-import.spec.ts | Adds test cases for the new rule enforcing namespace imports |
| src/rules/prefer-namespace-import.ts | Implements the rule logic with error reporting and auto-fix support |
| src/rules/prefer-default-export.ts | Updates the MessageId export for improved module interface |
| src/index.ts | Integrates the new rule into the exported rules |
| docs/rules/prefer-namespace-import.md | Provides documentation for the new rule |
| README.md | Updates the rule table with an entry for prefer-namespace-import rule |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #386 +/- ##
==========================================
+ Coverage 96.03% 96.05% +0.02%
==========================================
Files 95 96 +1
Lines 4917 4947 +30
Branches 1848 1855 +7
==========================================
+ Hits 4722 4752 +30
Misses 194 194
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 90c3115 in 1 minute and 46 seconds. Click for details.
- Reviewed
282lines of code in6files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rules/prefer-namespace-import.ts:78
- Draft comment:
Using substring extraction via indexOf('{') may be brittle for varied import formatting. Consider a token‐based approach to reliably capture the named specifiers. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The code is part of an ESLint rule's fixer, which needs to be robust. Using string manipulation could break with different formatting (spaces, newlines, comments). However, the code is getting the text directly from the AST node via sourceCode.getText(), which should be well-formed. The ESLint infrastructure likely ensures proper formatting. The suggested token-based approach would be more complex without clear benefits. The comment raises a valid concern about code robustness. String manipulation can be fragile compared to proper AST traversal. Since we're working within ESLint's infrastructure and getting text directly from AST nodes, the string manipulation approach is likely safe enough for this specific use case. While the comment raises a theoretically valid point, the current approach is pragmatic and likely robust enough given the context of ESLint's AST handling.
2. test/rules/prefer-namespace-import.spec.ts:45
- Draft comment:
Test coverage looks good but consider adding cases for combined default and named imports (and possibly type imports) to ensure the auto-fix handles all variants. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_JCMijP5ARV87TINU
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/rules/prefer-default-export.ts (1)
9-9: ExportingMessageIdwidens the public API surface
MessageIdwas previously an implementation detail of this rule; exporting it means it becomes part of the plugin’s public contract. If tests (or other rules) need the type, consider:
- Marking it
/** @internal */so tooling still hides it from users, or- Re-exporting it under a namespaced name (e.g.
PreferDefaultExportMessageId) to avoid potential clashes.Not blocking, but worth a second look.
README.md (1)
256-259: Verify that the rules table is regenerated, not hand-editedThe rules list is usually auto-generated. If this was a manual edit, run the docs generation script (
pnpm run docs:rulesor similar) to avoid future drift and keep the emoji columns consistent.docs/rules/prefer-namespace-import.md (1)
31-46: Minor doc clarity: highlight that regex entries must be stringsBecause JSON doesn’t allow bare regex literals, emphasise that regex patterns must be passed as strings (e.g.
"/^react-/"). Stating this explicitly avoids confusion.-`patterns` is an array of strings or `RegExp` patterns +`patterns` is an array of **strings**. +If a string starts and ends with `/`, it will be treated as a `RegExp` +pattern (e.g. `"/^react-/"`).test/rules/prefer-namespace-import.spec.ts (2)
7-10: RuleTester should declare the parser for ESLint v9 compatibility
@typescript-eslint/rule-testercurrently defaults to the TS parser, but ESLint v9 may require it to be set explicitly. Consider:-const ruleTester = new TSESLintRuleTester() +const ruleTester = new TSESLintRuleTester({ + parser: '@typescript-eslint/parser', +})
45-49: Add a “valid with patterns” case to complete coverageRight now the suite verifies:
- no-option (patterns = []) cases ✅
- option + invalid default import ❌
But it doesn’t assert that option + correct namespace import is accepted. Adding one keeps the regression net tight.
src/rules/prefer-namespace-import.ts (1)
67-70: Avoid.at(0)for broader Node-compatibility
String.prototype.atis only available from Node 16. Using bracket-indexing keeps the rule consumable by projects that are still on 14 LTS (which is still common in many CI environments).-const quote = node.parent.source.raw.at(0) ?? "'" +const quote = node.parent.source.raw?.[0] ?? "'"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)docs/rules/prefer-namespace-import.md(1 hunks)src/index.ts(2 hunks)src/rules/prefer-default-export.ts(1 hunks)src/rules/prefer-namespace-import.ts(1 hunks)test/rules/prefer-namespace-import.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/rules/prefer-namespace-import.spec.ts (1)
test/utils.ts (1)
createRuleTestCaseFunctions(143-158)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
🔇 Additional comments (3)
src/index.ts (2)
68-69: Import addition looks good and keeps alphabetical ordering
No issues spotted.
115-116: Rule registered correctly in the exported mapThe key follows the kebab-case convention and sits in the correct alpha slot after
prefer-default-export.src/rules/prefer-namespace-import.ts (1)
110-112: Defaulting to theuflag silently changes user regexesWhen a pattern string is of the form
/foo/, the code appends'u'if no flags are supplied.
That alters the behaviour of patterns that rely on legacy escapes or the.not matching astral symbols.Recommend leaving
flagsempty when the user didn’t specify any:-const [, pattern, flags = 'u'] = REGEXP_STR.exec(string) ?? [] +const [, pattern, flags = ''] = REGEXP_STR.exec(string) ?? []
react-x/prefer-namespace-importreact-x/prefer-react-namespace-import into prefer-namespace-import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 73bdcc9 in 1 minute and 0 seconds. Click for details.
- Reviewed
84lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/light-llamas-return.md:5
- Draft comment:
Clear changeset message; ensure the new rule name (import-x/prefer-namespace-import) is referenced consistently. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. test/rules/prefer-namespace-import.spec.ts:14
- Draft comment:
Good to see valid test cases using concise single-line code strings for namespace imports. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. test/rules/prefer-namespace-import.spec.ts:25
- Draft comment:
Valid test for non-matching modules (e.g. 'other-name') confirms default imports remain unaltered when patterns don't match. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. test/rules/prefer-namespace-import.spec.ts:36
- Draft comment:
Invalid tests correctly flag default and mixed (default + named) imports for modules matching the specified patterns. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. test/rules/prefer-namespace-import.spec.ts:61
- Draft comment:
Auto-fix output appropriately splits mixed default and named imports into separate statements while preserving order. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_5exwDHjsHhIqS4NS
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 6cec75c in 2 minutes and 32 seconds. Click for details.
- Reviewed
231lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rules/prefer-namespace-import.ts:45
- Draft comment:
Using 'ImportDefaultSpecifier' directly simplifies node matching. Verify that this selector catches all intended default import cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/rules/prefer-namespace-import.ts:64
- Draft comment:
For single default imports, replacing only the default specifier ensures that existing 'import' or 'import type' keywords are preserved. Confirm this behavior in all formatting cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/rules/prefer-namespace-import.ts:91
- Draft comment:
The toRegExp function's comment suggests plain strings become /^foo$/ but the implementation uses strict equality instead. Update the documentation to accurately reflect that behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. test/rules/prefer-namespace-import.spec.ts:18
- Draft comment:
Defining the constant 'options' for patterns improves maintainability. The test cases now comprehensively cover various import syntaxes including import assertions and TypeScript 'import type'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. test/utils.ts:56
- Draft comment:
Removal of default languageOptions injection simplifies test fixture setup. Confirm that tests still run with the intended parserOptions when not explicitly provided. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_MStqxJ84oBlS3QM3
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 7a6b4eb in 2 minutes and 25 seconds. Click for details.
- Reviewed
91lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rules/prefer-namespace-import.ts:61
- Draft comment:
When an import has additional named specifiers (e.g.import Foo, { bar } from 'foo'), the auto‐fix splits it into two separate import declarations. This transformation (lines 61–84) may produce duplicate module source clauses and can break code that expects a single import. Consider disabling the auto‐fix or implementing a fix that converts the entire import into a pure namespace import. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/rules/prefer-namespace-import.ts:104
- Draft comment:
The helpertoRegExpis acceptable overall, but if users supply an invalid regex string it silently falls back to simple equality. Consider adding validation or documentation that only valid regex strings (or plain strings) are supported. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_uXATLqJHcoHOPXzL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/rules/prefer-namespace-import.ts (1)
70-83: String slicing is still brittle for exotic import syntaxes (duplicate)
Previous reviews already highlighted that slicing on'{'/'}'can mis-identify the named-specifier block when decorators, comments, or exotic syntax (e.g.import Foo, { type Bar } from …) are present. Building the list fromnode.parent.specifiersis more robust.
🧹 Nitpick comments (1)
src/rules/prefer-namespace-import.ts (1)
105-109: Avoid silently adding theuregex flag
toRegExpforces theuflag when the user omits flags. This changes the semantics of simple patterns ('.'matches astral characters differently). Retain the user-supplied flags verbatim and only adduwhen none are provided and you need Unicode mode.-const [, pattern, flags = 'u'] = REGEXP_STR.exec(string) ?? [] +const [, pattern, flags = ''] = REGEXP_STR.exec(string) ?? []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)docs/rules/prefer-namespace-import.md(1 hunks)src/rules/prefer-namespace-import.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/rules/prefer-namespace-import.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/rules/prefer-namespace-import.ts (1)
src/utils/create-rule.ts (1)
createRule(70-70)
🪛 LanguageTool
README.md
[uncategorized] ~320-~320: You might be missing the article “a” here.
Context: ... | Prefer a default export if module exports a single name or multiple names...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/rules/prefer-namespace-import.ts (1)
61-68: Nice, minimal fix for default-only imports
The replacement keeps the original declaration intact while swapping the specifier — concise and safe.README.md (1)
301-322: Rule entry looks good – added row is correctly formatted, includes the 🔧 marker, and is placed in alphabetical order.
7a6b4eb to
0b6a12c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 0b6a12c in 1 minute and 7 seconds. Click for details.
- Reviewed
91lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rules/prefer-namespace-import.ts:72
- Draft comment:
The fixer for multiple specifiers uses string slicing (indexOf('{') and indexOf('}')) which may be brittle for complex formatting. Consider using AST node data for a more robust fix. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/rules/prefer-namespace-import.ts:104
- Draft comment:
The comment for 'toRegExp' states that normal strings are converted to /^foo$/, but the implementation returns a test function doing strict equality. Consider updating the comment or implementation for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_eM0l51lsxoMrkUjw
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 6e30b35 in 1 minute and 40 seconds. Click for details.
- Reviewed
90lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. test/rules/prefer-namespace-import.spec.ts:18
- Draft comment:
Added 'specific' pattern to options. Ensure literal string patterns are clearly documented as exact matches (unlike regex strings). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. test/rules/prefer-namespace-import.spec.ts:29
- Draft comment:
Valid test case for 'specific' using namespace import is added. Confirms that matching literal strings are not flagged. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
3. test/rules/prefer-namespace-import.spec.ts:50
- Draft comment:
Invalid test now includes a default import from 'specific'. This is correctly flagged and auto-fixed to a namespace import. Verify that the transform handles literal matches consistently. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the behavior of the code after a change, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue.
4. test/rules/prefer-namespace-import.spec.ts:122
- Draft comment:
TypeScript invalid test case now checks a type import from 'specific'. The auto-fix should convert it to 'import type * as Name5 from "specific"'. Confirm that type-specific syntax is handled correctly. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_BEbksQJFEjijVDcx
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/rules/prefer-namespace-import.ts (2)
72-83: String-slicing fix is still brittle; relies on first}occurrence
(See previous discussion – re-flagging for visibility.)If comments or string literals appear inside the named-import block the first
}might belong to them, producing malformed fixes:import Foo, { /* } */ Bar } from 'lib';Consider reconstructing the named specifiers directly from
node.parent.specifiersinstead of substring slicing. This avoids false matches and preserves formatting reliably.
104-110:toRegExpwill throw on invalid regex flags – confirm that fail-fast behaviour is acceptable
new RegExp(pattern, flags)will synchronously throw for invalid flag sets (e.g./foo/z).
Because the rule is created during linting, this will abort the entire ESLint run rather than reporting a configuration error.If the intent is truly “fail hard”, leave as is. Otherwise, wrap in a try/catch and surface an ESLint-style config error to the user.
🧹 Nitpick comments (2)
test/rules/prefer-namespace-import.spec.ts (2)
32-36: Duplicate local binding in “valid” fixture may break parsing
Name2is imported twice in the same module sample. ECMAScript modules forbid duplicate local bindings, and some parsers (especially Babel/Hermes) will throw a parse error before your rule executes, causing the test to fail for that parser.- import * as Name2 from 'prefix-name'; - import * as Name2 from 'specific'; + import * as Name2 from 'prefix-name'; + import * as Name3 from 'specific';Update the identifier (or remove the second line) to avoid parser-level failures.
90-97:parserConfigis ignored for the TS parser – loop iteration is redundant
@typescript-eslint/rule-testeralready sets@typescript-eslint/parseras the default parser.
Iterating overparsers.TStherefore re-runs the suite with exactly the same configuration, adding ~50 % test time for no extra coverage.If that duplication is unintentional, drop
parsers.TSfromgetNonDefaultParsers()or gate the loop withif (parser !== parsers.TS).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/light-llamas-return.md(1 hunks)README.md(1 hunks)docs/rules/prefer-namespace-import.md(1 hunks)src/index.ts(2 hunks)src/rules/prefer-default-export.ts(1 hunks)src/rules/prefer-namespace-import.ts(1 hunks)test/rules/prefer-namespace-import.spec.ts(1 hunks)test/utils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- test/utils.ts
🧰 Additional context used
🧠 Learnings (1)
src/rules/prefer-namespace-import.ts (1)
Learnt from: JounQin
PR: un-ts/eslint-plugin-import-x#386
File: src/rules/prefer-namespace-import.ts:41-46
Timestamp: 2025-06-18T15:22:38.492Z
Learning: In eslint-plugin-import-x, JounQin prefers to throw on invalid rule options rather than handling them gracefully with try/catch blocks and reporting configuration errors.
🧬 Code Graph Analysis (2)
src/rules/prefer-namespace-import.ts (2)
src/rules/prefer-default-export.ts (2)
Options(5-7)MessageId(9-9)src/utils/create-rule.ts (1)
createRule(70-70)
test/rules/prefer-namespace-import.spec.ts (1)
test/utils.ts (4)
createRuleTestCaseFunctions(135-150)getNonDefaultParsers(46-48)parsers(19-24)testFilePath(42-44)
🪛 LanguageTool
README.md
[uncategorized] ~320-~320: You might be missing the article “a” here.
Context: ... | Prefer a default export if module exports a single name or multiple names...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (6)
.changeset/light-llamas-return.md (1)
1-6: Changeset is correct
The changeset properly documents the newprefer-namespace-importfeature and targets a minor release. All fields and formatting follow the expected Changeset conventions.src/rules/prefer-default-export.ts (1)
9-9: ExposeMessageIdtype for external use
Exporting the previously internalMessageIdtype aligns this module with others and enables consistent typing in dependent code, without altering existing logic.src/index.ts (2)
68-68: Import newprefer-namespace-importrule
The rule is correctly imported alongside existing rules, using a consistent naming convention.
115-115: Registerprefer-namespace-importin the rules map
Adding the rule under the key'prefer-namespace-import'aligns with the plugin’s export structure and naming patterns.README.md (1)
321-321: The new table row forprefer-namespace-importmatches the style guide and uses the imperative form consistently.docs/rules/prefer-namespace-import.md (1)
1-7: Documentation is clear and comprehensive
The rule header, description, schema, and examples effectively illustrate configuration and usage, including autofix behavior.
close #258
related #385
https://eslint-react.xyz/docs/rules/prefer-react-namespace-import
Important
Introduces
prefer-namespace-importrule to enforce namespace imports for specified modules, with documentation and tests.prefer-namespace-importrule insrc/rules/prefer-namespace-import.tsto enforce namespace imports for specified modules.docs/rules/prefer-namespace-import.mdwith rule details and examples.README.mdto include the new rule in the rules list.prefer-namespace-importintest/rules/prefer-namespace-import.spec.tsto verify rule enforcement and autofix behavior.src/index.tsto include the new rule in the plugin's ruleset.This description was created by
for 6e30b35. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit