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

Merge imports #2603

Merged
merged 5 commits into from
Apr 12, 2018
Merged

Merge imports #2603

merged 5 commits into from
Apr 12, 2018

Conversation

topecongiro
Copy link
Contributor

This PR adds a merge_imports config option, which merges multiple imports with the same prefix into a single nested import.

Thoughts:

  1. We should not merge imports with different visibility or attributes.
  2. In the previous review, @nrc suggested to use different levels of merging over a simple boolean. I tried to use usize (0 means no merging, 1 means merging the top level segment, and so on), but it turned out to be kind of counter-intuitive IMO, so came back to true/false.

Closes #1383. Closes #2452. Closes #2544.

@topecongiro topecongiro changed the title Merge nested imports Merge imports Apr 6, 2018
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! A couple of small questions inline.

src/imports.rs Outdated

impl fmt::Debug for UseTree {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: think writing <Self as Display>::fmt(self, f) is clearer (and more efficient) than using write!

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be Display::fmt(self, f) too which is less verbose.

src/imports.rs Outdated
impl UseTree {
// Rewrite use tree with `use ` and a trailing `;`.
pub fn rewrite_top_level(&self, context: &RewriteContext, shape: Shape) -> Option<String> {
let mut result = String::with_capacity(256);
if let Some(ref attrs) = self.attrs {
result.push_str(&attrs.rewrite(context, shape)?);
result.push_str(&attrs.rewrite(context, shape).expect("rewrite attr"));
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? It seems that we could crash if the attribute can't be rewritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I changed this for debugging purpose, but I forgot to bring this back.

@@ -168,6 +241,17 @@ impl UseTree {
Some(result)
}

// FIXME: Use correct span?
Copy link
Member

Choose a reason for hiding this comment

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

Why is the span incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The span could be wrong when we are using the span from list imports. It should not matter, though, since we have already extracted comments around use items at this point.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add this detail to the comment please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure 😄

src/imports.rs Outdated
break;
}
}
if let Some(merged) = merge_rest(&self.path, &other.path, len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

len is always at new_path.len(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks!

@topecongiro
Copy link
Contributor Author

Ping @nrc 😄

@nrc nrc merged commit 55dd8f1 into rust-lang:master Apr 12, 2018
@nrc
Copy link
Member

nrc commented Apr 12, 2018

Thanks for the changes!

@topecongiro topecongiro deleted the merge-nested-imports branch April 12, 2018 04:44
@tikue
Copy link
Contributor

tikue commented Apr 27, 2018

Is it intended for this to put all std:: imports in a single block? e.g.

-use std::io;
-use std::marker::PhantomData;
-use std::net::{self, SocketAddr};
+use std::{io,
+          marker::PhantomData,
+          net::{self, SocketAddr}};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants