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

5796 derive attribute is not wrapped if it's exactly max length #6021

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

Conversation

IVIURRAY
Copy link
Contributor

#5796

This is not the prettiest solution but there is a assumption that once we enter write_list for list.rs we always subtract the separator from the width. This is causing the bug to occur here. As the line exceeds that max width by 1 and this subtraction makes the formatter think it is equal.

So the item_str we get back is actually greater than the width so I do a check for this and format it again but this time vertically.

I don't think this is pretty or a optimal fix (as we've to format twice) but I wanted to open a PR to generate a conversation about it. Maybe we can discuss a better solution to the problem now it has been identified a little more.

@IVIURRAY
Copy link
Contributor Author

IVIURRAY commented Jan 11, 2024

What I would really like to achive is getting definitive_tactic() to handle this correctly but that relys on the argument_shape which is correct.

rustfmt/src/attr.rs

Lines 133 to 138 in 6356fca

let tactic = definitive_tactic(
&all_items,
ListTactic::HorizontalVertical,
Separator::Comma,
argument_shape.width,
);

So which ever way I look at this, I always come back to these lines of code which are ignoring that final ,.

rustfmt/src/lists.rs

Lines 245 to 247 in 6356fca

let (sep_count, total_width) = calculate_width(items.clone());
let total_sep_len = sep.len() * sep_count.saturating_sub(1);
let real_total = total_width + total_sep_len;

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.

2 participants