Skip to content
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

Import ordering #2535

Merged
merged 8 commits into from
Mar 22, 2018
Merged

Import ordering #2535

merged 8 commits into from
Mar 22, 2018

Conversation

nrc
Copy link
Member

@nrc nrc commented Mar 16, 2018

cc #2185

This PR turns on import re-ordering by default and addresses some bugs in the implementation. Unfortunately that required quite a lot of re-writing.

r? @topecongiro

@nrc nrc mentioned this pull request Mar 16, 2018
6 tasks
Copy link
Contributor

@topecongiro topecongiro left a 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! I have not looked at the code in detail yet, just skimmed through the diffs and left some inline comments where I am not sure about.

@@ -14,16 +14,16 @@ extern crate env_logger;
extern crate getopts;
extern crate rustfmt_nightly as rustfmt;

use std::{env, error};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this and similar changes are correct. I feel that use std::{env, error}; should have higher precedence than use std::fs::File; and other imports that start with use std::.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I was pretty much making it up as I went along, but this was intentional. Looking at nested imports, I prefer use foo::{bar::baz, baz::{a, b}}; to the opposite ordering. I can't really justify why, but it felt to me like single imports should come before list imports. Do you have a reason for supporting the other order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually inside nested imports I prefer to put list imports after single imports, but on top level I prefer the opposite.

Maybe it is just that I am used to the current behavior. To keep the rule consistent I agree that we should put the list imports after single imports.

@@ -1300,13 +1300,12 @@ Reorder import statements in group
#### `false` (default):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you update the default value here (and in other places as well) please?

reorder_extern_crates_in_group: bool, true, false, "Reorder extern crate statements in group";
reorder_imports: bool, false, false, "Reorder import statements alphabetically";
reorder_imports_in_group: bool, false, false, "Reorder import statements in group";
reorder_extern_crates_in_group: bool, false, false, "Reorder extern crate statements in group";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the default value for reorder_extern_crates_in_group set to false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it is uncommon to group extern crates in the way that it is common to group use imports and therefore there is no need for the grouping and instead we should do what is simplest and gives the most consistent formatting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally feel that it is not that uncommon to group extern crates in some logical way (e.g. serde + log + rustc_private + local + others).

Also IMHO rerodering in groups gives the most consistent formatting, especially when #[macro_use] gets involved (we cannot reorder stuffs across items with #[macro_use]).

For example, with grouping disabled,

#[macro_use]
extern crate serde_derive;
extern crate serde_json;
extern crate serde;

extern crate semver;
extern crate toml;
extern crate itertools;

the above file will be formatted like this:

#[macro_use]
extern crate serde_derive;
extern crate itertools;
extern crate semver;
extern crate serde;
extern crate serde_json;
extern crate toml;

but if we put the serde group to the bottom,

extern crate semver;
extern crate toml;
extern crate itertools;

#[macro_use]
extern crate serde_derive;
extern crate serde_json;
extern crate serde;

it will be formatted like this:

extern crate itertools;
extern crate semver;
extern crate toml;
#[macro_use]
extern crate serde_derive;
extern crate serde;
extern crate serde_json;

@topecongiro
Copy link
Contributor

And needs a rebase. Sorry!

@nrc
Copy link
Member Author

nrc commented Mar 20, 2018

Rebased and with the change to grouping extern crates reverted.

@topecongiro
Copy link
Contributor

topecongiro commented Mar 21, 2018

Thank you for the update! LGTM. 😄

For further updates, if we want to handle rewriting and sorting at the same time, we need to extract comments around UseTree and UseSegment before sorting. Do you have a plan to do this? If not any time soon, I will merge this PR.

@nrc nrc merged commit c593229 into rust-lang:master Mar 22, 2018
@nrc
Copy link
Member Author

nrc commented Mar 22, 2018

For further updates, if we want to handle rewriting and sorting at the same time, we need to extract comments around UseTree and UseSegment before sorting. Do you have a plan to do this?

I don't have plans to do that, maybe as part of #2531 ?

@sbstp
Copy link
Contributor

sbstp commented Mar 23, 2018

There seems to be a mismatch between Configurations.md and the create_config! settings. All of the reorder settings are turned on by default now, but docs say that some of them are turned off.

@nrc
Copy link
Member Author

nrc commented Mar 28, 2018

I'll address that in a follow-up soon.

tanriol added a commit to tanriol/rustfmt that referenced this pull request Aug 29, 2019
The tests were modified in fa75ef4 (part of rust-lang#2535), probably
accidentally as the comments documenting the old expectations were kept.
tanriol added a commit to tanriol/rustfmt that referenced this pull request Mar 28, 2020
The tests were modified in fa75ef4 (part of rust-lang#2535), probably
accidentally as the comments documenting the old expectations were kept.
tanriol added a commit to tanriol/rustfmt that referenced this pull request Apr 10, 2020
The tests were modified in fa75ef4 (part of rust-lang#2535), probably
accidentally as the comments documenting the old expectations were kept.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants