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

Format with imports_granularity = "Module" #186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgeisler
Copy link
Contributor

Description of changes:

This is the result of running cargo +nightly fmt with the unstable rustfmt option imports_granularity = "Module".

Personally, I find this style much easier to use:

  • The lack of deep nesting makes it easier for me to understand the imports at a glance.
  • Because most imports are on a single line, I can add/remove them easier. Copy-pasting between files is easier.

Call-outs:

This option is unstable, meaning that it has no effect on a stable rustfmt. Luckily, the default value for imports_granularity is Preserve, which means that the formatting done here will be preserved by future cargo fmt calls:

https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#imports_granularity

This applies to both on the stable and nightly channels.

This is a personal preference, so this PR is just there to demonstrate the delta in case it hadn't been considered. Somewhat unintuitively, this style is more compact: the diffstat says 971 insertions(+), 1187 deletions(-), meaning that the imports take up 18% less vertical space now.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT license.

@mgeisler mgeisler requested a review from a team as a code owner August 19, 2024 09:33
mulmarta
mulmarta previously approved these changes Aug 20, 2024
Copy link
Contributor

@mulmarta mulmarta left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

For sure this makes the style consistent, which is already a big improvement.

I personally don't mind which style is used

@mgeisler
Copy link
Contributor Author

Cool, thanks!

For sure this makes the style consistent, which is already a big improvement.

Cool, I'm glad you like it! I'll fix the fuzz target imports too now.

This is the result of running `cargo +nightly fmt` with the unstable
`rustfmt` option `imports_granularity = "Module"`.

Personally, I find this style much easier to use:

- The lack of deep nesting makes it easier for me to understand the
  imports at a glance.
- Because most imports are on a single line, I can add/remove them
  easier. Copy-pasting between files is easier.

Of course, this option is unstable, meaning that it has no effect on a
stable `rustfmt`. Luckily, the default value for `imports_granularity`
is `Preserve`, which means that the formatting done here will be
preserved by future `cargo fmt` calls:

  https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#imports_granularity

This applies to both on the stable and nightly channels.
@mgeisler mgeisler force-pushed the imports-modularity-module branch from 2d57060 to e0ad168 Compare August 20, 2024 14:14
@mgeisler
Copy link
Contributor Author

I think we're getting closer — the fuzz targets are also formatted now. I thought they would get formatted by a cargo fmt --all, but I was wrong 😅

@stefunctional
Copy link
Contributor

This formatting style deviates from rust-analyzer's default, but that's probably a moot point as rust-analyzer seems to recognize the style and adapts automatic imports accordingly.

This style is inconsistent for conditional imports. This PR contains several blocks looking like this:

#[cfg(feature = "by_ref_proposal")]
use crate::{
    group::{
        framing::{Content, MlsMessagePayload},
        message_processor::CachedProposal,
        message_signature::AuthenticatedContent,
        proposal::Proposal,
        proposal_ref::ProposalRef,
        Sender,
    },
    WireFormat,
};

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