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

[Merged by Bors] - rustfmt groups imports #660

Closed
wants to merge 1 commit into from
Closed

Conversation

acrrd
Copy link
Contributor

@acrrd acrrd commented Oct 25, 2022

This enables the group_imports option. With this rustfmt automatically groups the imports in std, external, and `crate.
This is almost what we already wanted but we lose control of some things:

  • it is not able to differentiate between workspace and external so all our crates will go in that section,
  • it does not understand local import:
mod foo {
    struct Foo;
}

use foo::Foo;
use rand::random;

instead of having a separate group for foo.

  • It does not respect groups that we create manually
use tokio;

use itertools;

the imports will be merged.

Looking at the changeset we are not too bad at maintaining this style. Most of the changes are the position of crate wr.t. to self or super, fusing groups, and moving our workspace crates to the external one.
But this will be our only option to enforce this style with a tool.

https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#group_imports
rust-lang/rustfmt#4709
rust-lang/rustfmt#4693

@github-actions github-actions bot added the work-in-progress The author is still working on this PR label Oct 25, 2022
@janpetschexain
Copy link
Contributor

janpetschexain commented Oct 25, 2022

looks good imo as it helps with consistency and readability.

it is not able to differentiate between workspace and external so all our crates will go in that section,

that's not perfect, but more like a minor inconvenience.

it does not understand local import:

it's anyway more clear to use use self::foo::Foo; for local imports instead of use foo::Foo; to avoid any ambiguity for the reader. so it's good that rustfmt enforces the use of better style.

@acrrd acrrd force-pushed the rustfmt_group_imports branch from aa782ed to 45a82b1 Compare November 14, 2022 16:14
@acrrd acrrd marked this pull request as ready for review November 14, 2022 18:55
@acrrd acrrd requested a review from janpetschexain November 14, 2022 18:55
@github-actions github-actions bot added ready-for-review The PR can be reviewed and removed work-in-progress The author is still working on this PR labels Nov 14, 2022
Copy link
Contributor

@janpetschexain janpetschexain left a comment

Choose a reason for hiding this comment

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

lgtm

@janpetschexain janpetschexain removed the ready-for-review The PR can be reviewed label Nov 15, 2022
@acrrd
Copy link
Contributor Author

acrrd commented Nov 15, 2022

bors merge

self-hosted-bors bot pushed a commit that referenced this pull request Nov 15, 2022
This enables the `group_imports` option. With this rustfmt automatically groups the imports in `std`, `external`, and `crate.
This is almost what we already wanted but we lose control of some things:
* it is not able to differentiate between workspace and external so all our crates will go in that section,
* it does not understand local import: 
```rust
mod foo {
    struct Foo;
}

use foo::Foo;
use rand::random;
```
instead of having a separate group for `foo`. 
* It does not respect groups that we create manually
```rust
use tokio;

use itertools;
```
the imports will be merged.

Looking at the changeset we are not too bad at maintaining this style. Most of the changes are the position of `crate` wr.t. to `self` or `super`, fusing groups, and moving our workspace crates to the external one.
But this will be our only option to enforce this style with a tool.

https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#group_imports
rust-lang/rustfmt#4709
rust-lang/rustfmt#4693
@self-hosted-bors
Copy link
Contributor

Pull request successfully merged into main.

Build succeeded:

@self-hosted-bors self-hosted-bors bot changed the title rustfmt groups imports [Merged by Bors] - rustfmt groups imports Nov 15, 2022
@self-hosted-bors self-hosted-bors bot closed this Nov 15, 2022
@self-hosted-bors self-hosted-bors bot deleted the rustfmt_group_imports branch November 15, 2022 12:37
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.

2 participants