-
Notifications
You must be signed in to change notification settings - Fork 893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom import reordering #5632
base: master
Are you sure you want to change the base?
Custom import reordering #5632
Conversation
Hint: one may build a group for all crates within the workspace with cargo-metadata: WORKSPACE_INCLUDES=$(cargo metadata --offline --format-version 1 | jq -r '.workspace_members[]' | awk '{ print $1 }' | sed 's|-|_|' | sed 's|^\(.*\)$|"\1::\*",|' | tr '\n' ' ' | sed 's|, $||')
echo "[${WORKSPACE_INCLUDES}]" which for example for Rocket produces this: ["rocket::*", "rocket_codegen::*", "rocket_http::*", "rocket_db_pools_codegen::*", "rocket_db_pools::*", "rocket_sync_db_pools_codegen::*", "rocket_sync_db_pools::*", "rocket_dyn_templates::*", "rocket_guide_tests::*"] |
Wow, this is exactly what we need! Is there any updates about this?) |
@l4l I think you need to rebase your PR to get the tests working. |
cca9277
to
3b1d8b8
Compare
Found a failing test case: conf:
Input: use std::fmt;
use serde::de;
use workspace_p1::m1;
use workspace_p2::m2;
use workspace_p3 as p3;
use workspace_p4 as p4; Expected: use std::fmt;
use serde::de;
use workspace_p1::m1;
use workspace_p2::m2;
use {workspace_p3 as p3, workspace_p4 as p4}; Actual: use std::fmt;
use serde::de;
use {workspace_p3 as p3, workspace_p4 as p4};
use workspace_p1::m1;
use workspace_p2::m2; |
Will probably be useful to add a $pub which covers |
@utkarshgupta137 Thanks for continuing to help review this and for checking up on various test cases.
I don't want to extend the scope of this PR. We can consider adding that feature in a follow up PR |
For me it works as expected, it's not expected to match group_imports = [
["$std::*", "proc_macro::*"],
["*"],
["workspace*"],
] works indeed in a quite unexpected way (there's the same result). There's no simple way to split includes, reorder and then somehow "preserve" original granularity. I don't think this behavior need to be changed. The actual problem here is missing |
Is there any chance to get this reviewed and merged? |
Experience report: I tried using this option in a decently-sized personal project (~18 kLoC) with the following config: # ...
group_imports = [["*"], ["$crate::*"]]
# ... I only ran into one surprising "issue": local module imports were grouped with external crate imports: mod register;
mod view;
use glam::{const_vec3, Vec2, Vec3};
use mountain_river::high_scores::{HighScore, HighScores, HIGH_SCORES_PER_GROUP};
+ pub use register::RegisterHighScore;
use std::{array, borrow::Cow};
use triplicata::{
// ...
};
-
- pub use register::RegisterHighScore;
pub use view::ViewHighScores; Since this feature works by matching on import paths, this doesn't really seem like a bug. This was easily fixed by prepending Also, note that reexports were moved into a group containing normal imports. This is slightly unfortunate, but I believe that this is an acceptable price to pay for consistency. Otherwise, everything seems to have gone quite smoothly! |
a0371a5
to
bd08a3d
Compare
It seems this PR got forgotten, so kind reminder @ytmimi @calebcartwright If anything else you needed from me or have any concerns feel free to ping. If it's better to split-out the proc-macro part that's also not a problem. |
@l4l thanks for reaching out! I wouldn't say this has been forgotten, it's just been put on the back burner while we tend to some higher priority items. We spent a lot of time getting ready for In the interim, maybe you can add additional tests to see how this new option interacts with other import options like I'd also like to add that I understand this is important to you and many others, and I personally think it looks like a cool feature! |
Some random drive-by notes:
group_imports = [
["$std::*", "proc_macro::*"],
["*"],
["my_crate::*", "crate::*::xyz"],
["$crate::*"],
] I haven't looked at the algorithm but I assume it's trying to match from most specific to most general somehow. It would be more straightforward IMO to always match first to last, and then add a meta group_imports = [
["$std::.*", "proc_macro::.*"],
"$other",
["my_crate::.*", "crate::.*::xyz"],
["$crate::.*"],
] |
TLDR, for a given config:
The following order would be set:
This PR also contains three auxiliary patches for proc-macro
#[config_type]
:#[config_type(skip_derive(Debug, ...))]
, alsoimpl Copy
removed from most of types where unneeded;serde::{Serialize, Deserialize}
;impl Display
for singled-fielded enums.These are not really mandatory but simplify the final commit itself. Probably the proc macro could be refactored further but I think it shall be enough for now.
Implementation notes:
impl Deserialize for GroupImportsTactic
is required (Fails to (de)serialized enums with different variant types toml-rs/toml-rs#302Value
does not support enum deserialization from inline tables toml-rs/toml#364);escape(user_pattern).replace("\\*", ".*")
, performance should not be concern here anyway;group_imports
function (i.e forStdExternalCrate
) could be re-implemented withgroup_imports_custom
but I would rather prefer leaving it separate;$crate
alias doesn't mean usage of macro-specific$crate
import.Resolves #4693, resolves #4788, resolves #5550