Add ExternalCrate import grouping style
#5723
Conversation
|
Haven't looked at the proposed changes yet (and tbh it'll probably be a while before we can), but did want to go ahead and share that I'm generally receptive to incorporating this, and really any grouping style variant which is content with lumping all the things rustfmt can't deterministically recognize explicitly into an "other" bucket. One ask would be to ensure that there's additional sets of tests that include different combinations of the import related configuration options as these can sometimes sneakily step on each other (e.g. |
|
I've added the tests I think you were looking for. |
|
Apologies in advance for the bikeshed, but I feel like we'll want a better name than @ytmimi @fee1-dead do you have any thoughts/perspectives/suggested names? |
|
Will add that the code and tests lgtm, so I'd be content trying to get this merged and into the next release if we can come to an agreement on the variant name |
calebcartwright
added
pr-waiting-on-author
release-notes
| /// Divides imports into two groups, corresponding to external crates | ||
| /// and local imports. Sorts each subgroup. | ||
| fn group_imports_external_crate(uts: Vec<UseTree>) -> Vec<Vec<UseTree>> { | ||
| let mut external_imports = Vec::new(); | ||
| let mut local_imports = Vec::new(); | ||
|
|
||
| for ut in uts.into_iter() { | ||
| if ut.path.is_empty() { | ||
| external_imports.push(ut); | ||
| continue; | ||
| } | ||
| match &ut.path[0].kind { | ||
| UseSegmentKind::Ident(..) => external_imports.push(ut), | ||
| UseSegmentKind::Slf(_) | UseSegmentKind::Super(_) | UseSegmentKind::Crate(_) => { | ||
| local_imports.push(ut) | ||
| } | ||
| // These are probably illegal here | ||
| UseSegmentKind::Glob | UseSegmentKind::List(_) => external_imports.push(ut), | ||
| } | ||
| } | ||
|
|
||
| vec![external_imports, local_imports] | ||
| } | ||
|
|
There was a problem hiding this comment.
There could be a way to deduplicate this and StdExternalCrate, by either calling group_imports_std_external_crate in this function or calling this function in the other function.
There was a problem hiding this comment.
It's been a while since I was in the code, but I have a vague recollection that there wasn't much that was useful to DRY up here. That being said, WDYT about something that results in:
for ut in uts {
match UseCategory::categorize(ut) {
UseCategory::Std(ut) => std_imports.push(ut),
UseCategory::External(ut) => external_imports.push(ut),
UseCategory::Local(ut) => local_imports.push(ut),
}
}and
for ut in uts {
match UseCategory::categorize(ut) {
UseCategory::Std(ut) | UseCategory::External(ut) => external_imports.push(ut),
UseCategory::Local(ut) => local_imports.push(ut),
}
}
The things we qualify as |
|
Best I've ben able to come up with for now:
|
This creates two groups: 1. `std`, `core`, `alloc` and external crates, 2. `self`, `super` and `crate` imports.
shepmaster
force-pushed
the
external-crate-grouping
branch
from
6224160
to
ce29c9c
Compare
|
In my mind, I categorize it thusly: (further details on Zulip) So I'd probably prefer to call it something like
At the risk of shooting myself in the bikeshed, is there a better naming scheme that we could switch to (leaving the current name as an alias for compatibility)? One thing I've found tricky is the lack of words in between the nouns. As a introductory example that I have no attachment to, |
|
Yeah I was going to mention that we can change the name of the other variant in a non-breaking way if switching to a different naming convention allows for us to more easily derive a name that works for the new style introduced here |
This creates two groups:
std,core,allocand external crates,self,superandcrateimports.