Description
📋 Description
For the sake of example, lets say a project is composed of the following crates: coolapp-utils
, coolapp-backend
, and coolapp-frontend
It would be great if group_imports
supported a custom grouping between "external" crates (from crates.io) and "project local" crates:
// somewhere in `coolapp_frontend`
use alloc::alloc::Layout;
use core::f32;
use std::sync::Arc;
use chrono::Utc;
use uuid::Uuid;
use log::Level;
// At the moment, `group_imports = StdExternalCrate` fuses this group with the group above.
use coolapp_utils::frobnicate;
use coolapp_backend::api::ApiVersion;
use super::schema::{Context, Payload};
use super::update::convert_publish_payload;
use crate::models::Event;
Note that different projects have different notions of what constitutes a "project local" crate. Some examples I've come across in the past have included:
- workspace local crates
- crates that share a prefix, but are pulled from crates.io
- crates imported via a local relative path
- etc...
Since there wouldn't be any way to automatically determine which crates fall into which category, one reasonable implementation might be to modify the current "External" crate grouping to support being arbitrarily divided into two (or more?) groups.
While this wouldn't be a *perfect* solution (after all, users could still inadvertently import "project local" crates above "external" crates), it would be a step in the right direction, and offer a much needed bit of flexibility to group_imports
.
Activity
calebcartwright commentedon Feb 10, 2021
Thanks for reaching out with your request!
group_imports
is indeed a new option, but we knew even when adding initial support that there would likely eventually be various grouping strategies that folks would be interested in. The current variant is just what was including in the initial addition of the option, and no one has implemented any other variants sinceGenerally speaking, we're open to supporting any additional variants and associated groupings, but would have to rely on someone from the community to submit an implementation. The Style Guide, and thus rustfmt's default behavior, will continue to be a do-nothing/preserve on this front, so we can't really allocate any maintainer bandwidth into implementation.
The other thing I want to emphasize (as alluded to in the issue description) is that rustfmt's only source of information is the AST provided by rustc which would indeed limit the ability to actually implement the requested grouping.
I suppose an additional variant that attempted to take the original groupings into account could work in theory, but I imagine it could be more tricky than it may seem at first glance.
For example, consider this as input:
or this
Should there be two ext groups since there was a separation between them, or should
log::Level
be moved with the others in a single ext group since it was the only ext in a different group, and developer probably did that accidentally vs. trying to maintain two separate ext groups? Would the position in the original group make a difference? Or the quantity (e.g. two ext in the crate group)? Just some rhetorical examples of how there could potentially be a fair amount of edge cases here.The current variant isn't infinitely flexible, but it's quite predictable and deterministic which is essentially a requirement given the AST/pretty-printing nature of rustfmt. If anyone's interested in seeing this requested grouping and can come up with such an implementation though we'll be happy to review!
daniel5151 commentedon Feb 10, 2021
Thanks for the well thought out and detailed response @calebcartwright!
My *gut* reaction to those ambiguous cases would be for
rustfmt
to preserve groupings as best as possible, without worrying too much about the developer's intentions. e.g: for the second sample input, the output would be:While this output would most likely not be what the developer indented,
rustfmt
's output wouldn't be very "surprising". The rule would be simple, predictable, and deterministic, and could fill a role somewhere between the extremely rigidStdExternalCrate
option and not usinggroup_imports
at all.Yep, totally fair! I opened this issue since I just wanted to get some more visibility on the
group_imports
feature, and to check if other folks would be interested in seeing it extended.While I'd love to contribute this feature myself, Rust AST munging isn't something I've done in the past, so I might not be the right man for the job 😅. That said, who knows? Maybe I'll find some free time at some point to take a crack at it. After all,
group_imports
is definitely something I'd love to enable in my own projects :)calebcartwright commentedon Feb 12, 2021
Understood, it's just that there would be more of a "depends" factor with this variant vs.
StdExternalCrate
in terms of being able to describe what the result would because presence/absence of blank lines in the input will have an impact. That's not a problem nor a blocker, and we do this in other places as well, but it does bear emphasis.No worries! For you, or anyone interested in working on this, most of the relevant code can be found in
reorder.rs
andimports.rs
rustfmt/src/formatting/reorder.rs
Lines 201 to 206 in 98f2347
https://github.com/rust-lang/rustfmt/blob/master/src/formatting/imports.rs
jhpratt commentedon Feb 12, 2021
I'll throw out another possibility — why not make the option accept an array? Then users could put local imports first if they so desired, for example. There could be the "Std", "External", and "Crate" options that currently exist, with a "Project" (name TBD) being considered "External" unless explicitly specified. The first three could be mandatory.
calebcartwright commentedon Feb 12, 2021
Not sure I understand what you're suggesting @jhpratt, could you elaborate?
jhpratt commentedon Feb 12, 2021
In the above, "Crate" would include a proposed "Project", but it could be specified separately.
This would also let people reorder them however they'd like.
calebcartwright commentedon Feb 13, 2021
Thank you for the additional detail, though I confess that I'm still unclear on the relevance to this particular issue so please let me know if there's simply some dots i'm not connecting!
The type of the option as a string based array vs. enum variants is certainly a discussion we can have, but that feels orthogonal to (a) the fact that rustfmt can't actually make the "project" determination for the reasons discussed above and in other issues, and (b) the request for more granular/multi-groups for external.
There's a need for an implementation to be developed within rustfmt to support the requested style, regardless of whether the type of the option were to be changed; switching the option type from an enum to an array doesn't change that, and if anything would add complexity that would have be resolved first.
jhpratt commentedon Feb 13, 2021
Just another possibility that would provide more flexibility, I guess. Not 100% relevant to this request admittedly. I'll step aside, noting that I'd use this option if it were supported 🙂
calebcartwright commentedon Feb 13, 2021
No worries! If you'd like to continue the discussion then I think a new issue would be best, to support respective focused conversations
group_imports
does not differentiate between a local import and an external crate #4709group_imports
#50836 remaining items