feat(organizeImports): add sortBareImports option#9384
Conversation
🦋 Changeset detectedLatest commit: fa981cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a new organizeImports option Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
05f6776 to
887b702
Compare
8a3a783 to
cadf73d
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
I like it! |
| let sort_bare_imports = options.sort_bare_imports.unwrap_or_default(); | ||
| let mut chunk: Option<ChunkBuilder> = None; | ||
| let mut prev_kind: Option<JsSyntaxKind> = None; | ||
| let mut prev_is_bare_import = false; |
There was a problem hiding this comment.
I am not fond of this new variable. However, the alternative approach I have in mind is not necessarily better: The alternative implementation is to modify ImportInfo::from_module_item to return None if sortBareImports is false/unset and returns the info if it is true. However, this led to some code duplication in the else branch: we have to check whether the attributes of bare imports are sorted.
Thus, I decided to keep the current approach.
| @@ -0,0 +1,4 @@ | |||
| /* should not generate diagnostics */ | |||
There was a problem hiding this comment.
This is a non-regression test: While I was implementing the feature I introduced a bug that was not covered by the test suite. This test covers it.
|
The lint rule docs CI task fails. However, this seems to come from other rules. Thus, I think we can ignore this failure. |
sortBareImports option
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_js_analyze/src/assist/source/organize_imports.rs`:
- Line 67: Update the doc comment that documents sortBareImports so the sentence
starts with a capital letter: change "when `sortBareImports` is unset or
`false`, every bare import forms an independent chunk." to "When
`sortBareImports` is unset or `false`, every bare import forms an independent
chunk." in the organize_imports.rs documentation for sortBareImports.
- Line 674: Fix the typo in the documentation comment in organize_imports.rs
where the string "Yo ucan opt for a `lexicographic` sort (sometimes referred as
_binary_ sort) by" contains a stray space; change "Yo ucan" to "You can" in that
doc comment (search for the exact phrase "Yo ucan opt for a `lexicographic`
sort" to locate the comment).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21306b67-39f2-42ed-98af-479c5a1d0bfd
⛔ Files ignored due to path filters (2)
packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (4)
.changeset/flat-beers-battle.mdcrates/biome_js_analyze/src/assist/source/organize_imports.rscrates/biome_js_analyze/tests/specs/source/organizeImports/custom-order-with-bare-imports.options.jsoncrates/biome_rule_options/src/organize_imports.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/flat-beers-battle.md
- crates/biome_js_analyze/tests/specs/source/organizeImports/custom-order-with-bare-imports.options.json
Summary
This PR adds a new option
ignoreBareImportssortBareImportsthat allows bare imports to be sorted within other imports when set tofalsetrue.Example:
{ "assist": { "actions": { "source": { "organizeImports": { "level": "on", "options": { "sortBareImports": true } } } } } }As you noticed, I chose to not merge bare imports with other imports because a bare import signals a side effect. Merging them could remove this signal.
I am a bit hesitant about the option name.I chose
ignoreBareImports. However, bare imports are not completely ignored because we sort their attributes.An alternative name could be
sortBareImports. Any opinion?EDIT: I switched to
sortBareImports. This looks better to me.Note that this PR doesn't address https://github.com/biomejs/bioze/discussions/7811 yet.
I plan to submit a future PR that allows matching against bare imports.
This will require to set
ignoreBareImportstofalse.The PR fixed also a small bug: import attributes of bare imports were not sorted.
Test Plan
I added several tests to cover the new option.
Docs
I updated the rule docs and I added a changeset.
Also I added doc-comment to the options.