Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Option to create groups for std, external crates, and other imports #4445
Conversation
|
Thanks for the PR @MattX! Excited to see this one. I don't think I'll have a chance to give this a review tonight, but a few initial thoughts for you:
|
The bug with comment association will be a blocker for being able to stabilize this option, but not a blocker for being able to make it available on nightly as an unstable option |
MattX
changed the title
|
Thanks for the feedback!
The way I've implemented it, this new option has no effect when In any case, it might make sense to add a warning or error when |
|
Thanks for the updates, I like the new name better.
I was wondering about this one. I don't think we'll be able to couple the two options together and have this one only be operational one another has a certain value. Logically it makes sense that the majority of folks that would want an enforced grouping (and a sorting of those groups) would want to have the elements sorted within that group, but it's almost inevitable that there will be some folks that want to use their own/different sorting within the groups. I imagine it'll add a bit of complexity but it's something we'll need to do (Sorry for the PR close/reopen, accidentally hit the wrong button) |
| ## `group_imports` | ||
|
|
||
| Discard existing import groups, and create three groups for: | ||
| 1. `std`, `core` and `alloc`, | ||
| 2. external crates, | ||
| 3. `self`, `super` and `crate` imports. | ||
|
|
||
| Within each group, imports are sorted as with `reorder_imports`. | ||
|
|
||
| This has no effect is `reorder_imports` is `false`. | ||
|
|
||
| - **Default value**: `None` | ||
| - **Possible values**: `None`, `StdExternalCrate` | ||
| - **Stable**: No | ||
|
|
||
| #### `None` (default): |
calebcartwright
Oct 4, 2020
Member
Let's keep the description of the option generic about what the option does/what it controls as opposed to describing the behavior of a particular supported variant (e.g. Controls the strategy for how imports are grouped together...)
And given the denotations of None within Rust, I'd suggest we use Preserve instead, which will also be consistent with a few other rustfmt config option variants
Let's keep the description of the option generic about what the option does/what it controls as opposed to describing the behavior of a particular supported variant (e.g. Controls the strategy for how imports are grouped together...)
And given the denotations of None within Rust, I'd suggest we use Preserve instead, which will also be consistent with a few other rustfmt config option variants
|
Thanks for the feedback! I was actually able to decouple ordering and grouping without much difficulty. I've also renamed |
| @@ -78,6 +78,8 @@ create_config! { | |||
| imports_indent: IndentStyle, IndentStyle::Block, false, "Indent of imports"; | |||
| imports_layout: ListTactic, ListTactic::Mixed, false, "Item layout inside a import block"; | |||
| merge_imports: bool, false, false, "Merge imports"; | |||
| group_imports: GroupImportsTactic, GroupImportsTactic::Preserve, false, | |||
| "Reorganize import groups"; | |||
calebcartwright
Oct 6, 2020
Member
Minor nit but could we update this with something similar to what was changed in the Configurations.md file, like Controls the strategy for how imports are grouped together?
Minor nit but could we update this with something similar to what was changed in the Configurations.md file, like Controls the strategy for how imports are grouped together?
| @@ -227,19 +229,34 @@ fn rewrite_reorderable_items( | |||
| if context.config.merge_imports() { | |||
| normalized_items = merge_use_trees(normalized_items); | |||
| } | |||
| normalized_items.sort(); | |||
calebcartwright
Oct 6, 2020
Member
I'll confess this gave me pause at first glance, but see why we had to wrap the sorting behind the reorder_imports check here now since we can get here even if reorder_imports false with non-Preservation group import strategies.
I'll confess this gave me pause at first glance, but see why we had to wrap the sorting behind the reorder_imports check here now since we can get here even if reorder_imports false with non-Preservation group import strategies.
|
Excellent work @MattX, thank you! One super minor item left inline, but otherwise this is good to go from my perspective |
MattX
force-pushed the
MattX:master
branch
2 times, most recently
from
0da8517
to
ee1acb5
MattX
changed the title
|
|
|
Everything LGTM to me @MattX. Going to hold off a bit on merging though so @topecongiro can weigh in since this is a new config option |
|
Sounds good! |
|
Once it's reviewed, any chance this can be tagged as hacktoberfest-accepted? I want my t-shirt :p |
|
Thank you for the PR! I think that the current implementation is good enough for the first round of implementing this feature, except for the inline comment. Would you mind fixing it and adding a test? |
|
|
||
| wrap_reorderable_items(context, &item_vec, nested_shape) | ||
| Some(item_vec.join("\n\n")) |
topecongiro
Oct 14, 2020
Collaborator
We need to respect the current indentation here to indent inside inner modules correctly. E.g.,
mod test {
use crate::foo::bar;
use std::path;
}
We need to respect the current indentation here to indent inside inner modules correctly. E.g.,
mod test {
use crate::foo::bar;
use std::path;
}
MattX
Oct 27, 2020
Author
Contributor
Good catch, I've added code to handle this case but I don't know if it's the best way to do things. Let me know if it's not.
Good catch, I've added code to handle this case but I don't know if it's the best way to do things. Let me know if it's not.
|
Looks like the |
|
There's a broken toolstate issue on nightly at the moment that requires us to bump the rustc-ap crates here. Those crates (snapshots of rustc internals) are published once a week, and I'm waiting to grab the next one which should be done in ~16 hours or so. |
|
crates bumped, you should be able to rebase now |
It might actually be better to have a three-way option.
Tests for interaction with `no_reorder` and `merge_imports`.
Also reword configuration to be less specific.
|
Hey, just pinging on this to make sure it gets merged before it's too outdated. Let me know if there's anything that still needs to be done on my end. |
|
@topecongiro going to go ahead and merge as the change you requested appears to have been resolved |
calebcartwright
merged commit 17d90ca
into
rust-lang:master
|
@MattX - note that while this has been merged, and thus immediately available for anyone building rustfmt 2.0 from source, it's not available in the version of rustfmt distributed via rustup as that's on a different branch. If you'd like to see this new feature supported in a released version of rustfmt, that will need to be backported to a target 1.x branch (1.4.27 at the moment). If you're interested in such a PR please let us know, otherwise I'll try to take a look at cherry-picking/backporting at some point down the road |
|
Ok! I'm opening a new PR for 1.4.27. |
This adds a configuration flag,
reorder_imports_opinionated, that causes existing import groups to be discarded and replaced by three groups for standard imports, other imports, andself/super/crateimports.The option name isn't great, we can change it if there are better ideas.
I know @calebcartwright raised a possible issue in #4107 (comment), but I'm not sure if it is still a blocker.
Resolves #4107.