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

Use the same shape to rewrite import when reordering them or not #5307

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

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Apr 11, 2022

Fixes #4794

The code paths for formatting imports diverge based on the value of
reorder_imports. Each code path used a slightly different shape when
rewriting the imports which lead to issues when reorder_imports = false
and imports_indent = Visual.

Now the code path that rewrites imports without reordering uses the same
shape as the path that also reorders imports.

Comment on lines +34 to +43
// 4 = "use ", 1 = ";"
let nested_shape = shape
.offset_left(4)
.and_then(|s| s.sub_width(1))
.unwrap_or(shape);
Copy link
Contributor Author

@ytmimi ytmimi Apr 11, 2022

Choose a reason for hiding this comment

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

This is the same shape that gets passed to rewrite_top_level from rewrite_reorderable_or_regroupable_items when reorder_imports=true

rustfmt/src/reorder.rs

Lines 131 to 146 in 2d9bc46

// 4 = "use ", 1 = ";"
let nested_shape = shape.offset_left(4)?.sub_width(1)?;
let item_vec: Vec<_> = regrouped_items
.into_iter()
.filter(|use_group| !use_group.is_empty())
.map(|use_group| {
let item_vec: Vec<_> = use_group
.into_iter()
.map(|use_tree| ListItem {
item: use_tree.rewrite_top_level(context, nested_shape),
..use_tree.list_item.unwrap_or_else(ListItem::empty)
})
.collect();
wrap_reorderable_items(context, &item_vec, nested_shape)
})
.collect::<Option<Vec<_>>>()?;

Now we're using this shape when we call rewrite_top_level from format_import.

@calebcartwright
Copy link
Member

I think this is good, but could you add some test cases that have imports nested a few levels (e.g. imports within some in-line mod or other item)

Fixes 4794

The code paths for formatting imports diverge based on the value of
`reorder_imports`. Each code path used a slightly different shape when
rewriting the imports which lead to issues when `reorder_imports = false`
and `imports_indent = Visual`.

Now the code path that rewrites imports without reordering uses the same
shape as the path that also reorders imports.
@ytmimi
Copy link
Contributor Author

ytmimi commented Jun 22, 2022

could you add some test cases that have imports nested a few levels (e.g. imports within some in-line mod or other item)

@calebcartwright I added the test cases you were looking for. CI is still running right now, but assuming it passes I think we're good to go here!

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

Successfully merging this pull request may close these issues.

reorder_imports=false conflicts with imports_indent="Visual"
2 participants