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

imports_granularity: implement One/Single variant #4669

Closed
calebcartwright opened this issue Jan 24, 2021 · 2 comments · Fixed by #4680
Closed

imports_granularity: implement One/Single variant #4669

calebcartwright opened this issue Jan 24, 2021 · 2 comments · Fixed by #4680
Labels
1x-backport:completed a-imports `use` syntax feature-request good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted

Comments

@calebcartwright
Copy link
Member

Add a new variant and associated formatting to imports_granularity that reformats all imports into a single use statement. The variant name should be One or Single, with One being the leading contender (refer here for One and here for Single for respective community votes/feedback)

use {
    futures::{
        ready,
        stream::{Stream, StreamExt},
    },
    std::pin::{Pin, Unpin},
};

Background

imports_granularity is a configuration option that allows users to control the granularity of their use declarations (https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#imports_granularity)

By default rustfmt will not modify the granularity and will preserve the developers original structure, but the imports_granularity option provides several different variants that users can leverage to instruct rustfmt to reformat their use statements with a specific level of granularity.

Several different styles/granularity levels have been identified (summarized here, and already implemented, with the exception of the one/single granularity variant.

Implementation

There will be 3 high level tasks required here

  • Updating the configuration option information
  • Updating the imports merging logic to support the new granularity style
  • Adding tests

For the configuration side:

For implementation, refer to relevant sections in src/formatting/imports.rs and src/formatting/reorder.rs, some suggested starting points:

pub(crate) enum SharedPrefix {
Crate,
Module,
}

fn share_prefix(&self, other: &UseTree, shared_prefix: SharedPrefix) -> bool {
if self.path.is_empty()
|| other.path.is_empty()
|| self.attrs.is_some()
|| !self.same_visibility(other)
{
false
} else {
match shared_prefix {
SharedPrefix::Crate => self.path[0] == other.path[0],
SharedPrefix::Module => {
self.path[..self.path.len() - 1] == other.path[..other.path.len() - 1]
}
}
}
}

normalized_items = match context.config.imports_granularity() {
ImportGranularity::Crate => merge_use_trees(normalized_items, SharedPrefix::Crate),
ImportGranularity::Module => {
merge_use_trees(normalized_items, SharedPrefix::Module)
}
ImportGranularity::Item => flatten_use_trees(normalized_items),
ImportGranularity::Preserve => normalized_items,
};

For tests (more information on writing tests here):
Some unit tests for the other variants can be found here
https://github.com/rust-lang/rustfmt/blob/master/src/formatting/imports.rs#L1032-L1087

Additional files for the system/idempotence tests can be found under tests/{src,target}/imports_granularity_{crate,item,module}.rs for reference

@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors feature-request a-imports `use` syntax help wanted 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release labels Jan 24, 2021
@magurotuna
Copy link
Contributor

I'm interested in this issue. Can I work on it?

@calebcartwright
Copy link
Member Author

I'm interested in this issue. Can I work on it?

Absolutely! Let us know if you have any questions

magurotuna added a commit to magurotuna/rustfmt that referenced this issue Jan 31, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
magurotuna added a commit to magurotuna/rustfmt that referenced this issue Jan 31, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
magurotuna added a commit to magurotuna/rustfmt that referenced this issue Jan 31, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
magurotuna added a commit to magurotuna/rustfmt that referenced this issue Mar 20, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
magurotuna added a commit to magurotuna/rustfmt that referenced this issue Mar 20, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
magurotuna added a commit to magurotuna/rustfmt that referenced this issue Mar 24, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
magurotuna added a commit to magurotuna/rustfmt that referenced this issue Mar 24, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
calebcartwright pushed a commit that referenced this issue Mar 25, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
irh pushed a commit to irh/rustfmt that referenced this issue Aug 31, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
calebcartwright pushed a commit that referenced this issue Aug 31, 2021
This option merges all imports into a single `use` statement as long as
they have the same visibility.
@calebcartwright calebcartwright added 1x-backport:completed and removed 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release labels Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1x-backport:completed a-imports `use` syntax feature-request good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants