feat(lint): add nursery rule useImportsFirst#9272
feat(lint): add nursery rule useImportsFirst#9272terror wants to merge 11 commits intobiomejs:mainfrom
useImportsFirst#9272Conversation
🦋 Changeset detectedLatest commit: a784b75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
WalkthroughAdds a new nursery lint rule Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid.js (1)
1-5: Optional: add a directive-focused valid fixture.Given the rule docs explicitly allow directives, a tiny
valid_with_directive.js(e.g."use strict";then imports) would lock that behaviour in and guard regressions.Based on learnings: Add tests for all code changes - lint rules need snapshot tests in
tests/specs/{group}/{rule}/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid.js` around lines 1 - 5, Add a new valid fixture file (e.g., valid_with_directive.js) under tests/specs/nursery/useImportsFirst/ that begins with a directive like "use strict"; followed by the same import lines (import { foo } from "foo"; import { bar } from "bar"; import { baz } from "baz"; const qux = 1;) and the same top comment ("should not generate diagnostics") so the test suite captures that directives are allowed by the useImportsFirst rule; ensure the file follows the existing snapshot test pattern used in tests/specs/{group}/{rule}/ so it is picked up by the rule tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_rule_options/src/use_imports_first.rs`:
- Line 6: Add a rustdoc comment above the public struct UseImportsFirstOptions
explaining what this options type configures (options for the
"use-imports-first" rule/assist), what each field would control (if currently
empty, state that it has no configurable fields yet and is reserved for future
settings), and include example usage or default behavior; ensure the doc uses
standard /// rustdoc format placed immediately before the declaration of
UseImportsFirstOptions.
---
Nitpick comments:
In `@crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid.js`:
- Around line 1-5: Add a new valid fixture file (e.g., valid_with_directive.js)
under tests/specs/nursery/useImportsFirst/ that begins with a directive like
"use strict"; followed by the same import lines (import { foo } from "foo";
import { bar } from "bar"; import { baz } from "baz"; const qux = 1;) and the
same top comment ("should not generate diagnostics") so the test suite captures
that directives are allowed by the useImportsFirst rule; ensure the file follows
the existing snapshot test pattern used in tests/specs/{group}/{rule}/ so it is
picked up by the rule tests.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/invalid_mixed.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid_with_export.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (7)
crates/biome_js_analyze/src/lint/nursery/use_imports_first.rscrates/biome_js_analyze/tests/specs/nursery/useImportsFirst/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useImportsFirst/invalid_mixed.jscrates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid.jscrates/biome_js_analyze/tests/specs/nursery/useImportsFirst/valid_with_export.jscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_imports_first.rs
|
dyc3
left a comment
There was a problem hiding this comment.
Looks good, just a couple notes
Merging this PR will not alter performance
Comparing Footnotes
|
Netail
left a comment
There was a problem hiding this comment.
Would also suggest adding a test with a directive at the beginning of a file
| rule_category!(), | ||
| range, | ||
| markup! { | ||
| "This import should appear at the top of the module." |
There was a problem hiding this comment.
This message doesn't align with the rule pillars. The message must tell what's the error, not the solution. It should say something like "This import shouldn't be here" or something.
There was a problem hiding this comment.
Just changed this up. I believe I now follow what 'what', 'why' and 'how' structure we're aiming for.
Summary
Resolves #9265
This diff Implements the
useImportsFirstlint rule, which enforces that all import statements appear before any non-import statements in a module. This corresponds to the ESLint import/first rule (see #9265). The rule iterates over the module item list and flags anyJsImportthat appears after a non-import item.Test Plan
Snapshot tests in
crates/biome_js_analyze/tests/specs/nursery/useImportsFirst/:invalid.js- imports after a const declaration (2 diagnostics)invalid_mixed.js- imports interspersed with calls and exports (2 diagnostics)valid.js- all imports grouped at the top (no diagnostics)valid_with_export.js- single import before export and statement (no diagnostics)Docs
Rule documentation is inline in the
declare_lint_rule!macro and will be published automatically.