-
Notifications
You must be signed in to change notification settings - Fork 925
Coalesce some of the import options #2605
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! And my apologies for the late review. The code looks good other than the minor refactoring. Also, I think that we should move the visual guides and unit tests of removed config options to reorder_imports
section, rather than removing them entirely.
src/reorder.rs
Outdated
ReorderableItemKind::ExternCrate => config.reorder_extern_crates_in_group(), | ||
ReorderableItemKind::Mod => config.reorder_modules(), | ||
ReorderableItemKind::Use => config.reorder_imports_in_group(), | ||
ReorderableItemKind::ExternCrate => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These patterns can be combined into a single pattern.
Configurations.md
Outdated
|
||
## `reorder_imports` | ||
|
||
Reorder import statements alphabetically | ||
Reorder import and extern crate statements alphabetically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mention that statements are reordered "in group"? Also, if possible, we should move the examples of removed options under reorder_imports
section, rather than removing them entirely.
Default value changed in rust-lang#2605
Default value changed in #2605
cc #2185
r? @topecongiro