-
Notifications
You must be signed in to change notification settings - Fork 898
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
leverage itemize_list
and write_list
to recover trait bound comments
#6048
base: master
Are you sure you want to change the base?
Conversation
> + fmt::Write | ||
> | ||
+ fmt::Write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look at the style guide description for traits
emphasis mine:
Prefer not to line-break in the bounds if possible (consider using a where clause). Prefer to break between bounds than to break any individual bound. If you must break the bounds, put each bound (including the first) on its own block-indented line, break before the + and put the opening brace on its own line:
I think these changes align more closely with the language in the guide than the formatting that we had before, but please let me know if I'm misinterpreting the guide.
} else { | ||
indented.indent.to_string_with_newline(&context.config) | ||
}; | ||
result.push(':'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we apply space_before_colon
?
// `offset_left` takes into account what we've rewritten already + 1 for `:` | ||
// `sub_width` take into account the trailing `{` | ||
shape.offset_left(header.width() + 1)?.sub_width(1)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to be header.width() + 2
if we're applying space_before_colon
, Also I'm realizing that the header doesn't contain any rewritten generics. Might be better to move the shape derivation into rewrite_bounds
Fixes 2055 Now when using `version::Two`, rustfmt will permit comments within trait bounds and actually reformat them instead of refusing to rewrite the entire trait definition because there are comments in the bounds.
TODO: add test case for #5321, which I think this also fixes |
I tried a bit, I think we can do better for supertrait: I have this code: pub trait MyLongTraitName:
MyLongBoundName<
MyLongAssociatedType:
SomeAnotherBound<
YetAnotherType: YetAnotherBound,
YetAnotherType2: YetAnotherBound2
>,
OtherType: SmallBound
+ SmallBound2
+ AnotherLongBound<
WithSomeType: BeingBound,
WithSomeType2: BeingBound,
WithSomeType3: BeingBound
>
+ SmallBound3
+ AnotherLongBound2<
WithSomeType: BeingBound,
WithSomeType2: BeingBound,
WithSomeType3: BeingBound
>,
>
{} It gets formatted into this with this PR: pub trait MyLongTraitName:
MyLongBoundName<
MyLongAssociatedType: SomeAnotherBound<
YetAnotherType: YetAnotherBound,
YetAnotherType2: YetAnotherBound2,
>,
OtherType: SmallBound
+ SmallBound2
+ AnotherLongBound<
WithSomeType: BeingBound,
WithSomeType2: BeingBound,
WithSomeType3: BeingBound,
> + SmallBound3
+ AnotherLongBound2<
WithSomeType: BeingBound,
WithSomeType2: BeingBound,
WithSomeType3: BeingBound,
>,
>
{
} we can see the generics inside |
Fixes #2055
Now when using
version::Two
, rustfmt will permit comments within trait bounds and actually reformat them instead of refusing to rewrite the entire trait definition because there are comments in the bounds.This follows the approach hinted at in #2055 (comment).
Edit -- Other issues fixed by this change:
Fixes #6127