feat(rome_js_analyze): implement the organizeImports code action #3818
Conversation
✅ Deploy Preview for docs-rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| /// ``` | ||
| pub(crate) OrganizeImports { | ||
| version: "11.0.0", | ||
| name: "organizeImports", |
There was a problem hiding this comment.
Should the name be useOrganizedImports?
There was a problem hiding this comment.
Ironically, we're not applying this pattern to assists, but I think we should too.
There was a problem hiding this comment.
Currently assist rules have a different naming convention from lint rules, since they're primarily about exposing an action they're using an imperative phrasing for the action they perform: "flip binary expression", "inline variable" or "organize imports"
There was a problem hiding this comment.
This is true, but this could confuse the users because they are not aware of this distinction (and we don't actually document assists yet, and I bet the majority of the users are not aware of them).
We could keep the current name, but we need to come up with:
- a way to document assists on our website (not a big deal)
- explain to the users why this feature is here and it's not a formatter feature
- explain to the users and us how and when some assists can opt-in via CLI (at the moment assists are only available via LSP)
There was a problem hiding this comment.
I've started looking a bit into this, and I think that's probably better handled in separate PRs. Specifically:
- An update to the documentation and website to better document the "editor actions" we currently support (I just made up that name, it includes the analyzer-based assist rules along with the unstable rename action)
- Adding a way to generally configure import sorting in the
rome.jsonfile, and that would specifically include an option to either run the assist as part ofrome checkorrome format
| let mut iter = (&mut items_iter).enumerate(); | ||
|
|
||
| // Iterate other the nodes of the old list | ||
| while let Some((item_slot, item)) = iter.next() { |
There was a problem hiding this comment.
Nit: It could be useful to provide mutation methods on lists that replace an element by its index to avoid iterating over all elements (and having to allocate a new vec because of that)
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct ImportKey(SyntaxTokenText); |
There was a problem hiding this comment.
I don't see a reason why SyntaxTokenText shouldn't implement Ord, PartialOrd. It may reduce some of the boilerplate that you had to write here.
There was a problem hiding this comment.
I agree it would make sense to have SyntaxTokenText implement Ord and PartialOrd, but I still think we could keep the ImportKey type in order to implement a specific ordering logic for imports (the many of the popular import sorting plugins for Prettier use a natural sorting algorithm instead of alphabetical)
There was a problem hiding this comment.
Totally, I like the ImportKey, even just to have some place for documenting the ordering (and what the key is, SyntaxTokenText could be any token)
crates/rome_js_analyze/src/assists/correctness/organize_imports.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/assists/correctness/organize_imports.rs
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,331 @@ | |||
| use std::{ | |||
There was a problem hiding this comment.
Even though it's not a rule but an assists, I think we should apply the same convention we do for rules. Start from the nursery group and then promote it.
There was a problem hiding this comment.
I was actually considering a way to remove groups completely from the assists category, since they only do anything for lint rules
| /// ``` | ||
| pub(crate) OrganizeImports { | ||
| version: "11.0.0", | ||
| name: "organizeImports", |
There was a problem hiding this comment.
Ironically, we're not applying this pattern to assists, but I think we should too.
crates/rome_js_analyze/src/assists/correctness/organize_imports.rs
Outdated
Show resolved
Hide resolved
|
Update: I've improved the way the rule handles trivia to avoid having to create (or remove) newlines trivia pieces by recycling the trivia from existing tokens instead. I've also used this refactor to implement support for file-level comments by preserving the order of some trivia pieces that are separated from the rest of the group by an empty line, for instance: // this comment will stay in place
// this comment will be moved with the import below
import b from 'b';
import a from 'a'; |
|
@leops IIUC this patch doesn't enable users to apply sorting via CLI. Is there plans to extend the CLI to allow for that? |
|
Thanks @Conaclos! I've tried |
Summary
Fixes #3462
This is an initial implementation of import sorting, exposed through the
source.organizeImportscode action. For now, the grouping of individual imports is not configurable and the rule will only reorder imports within the same "group", blocks of adjacent import nodes separated by empty lines or non-import module items (export or statement nodes). The rule also makes no attempts at merging imports from the same source (these will be preserved as individual imports and retain their original order relative to each other).Test Plan
I've added a few snapshot tests that should cover everything that's currently supported by this version of the rule