-
Notifications
You must be signed in to change notification settings - Fork 928
Allow generic arguments in chain method calls to overflow #5252
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
base: master
Are you sure you want to change the base?
Conversation
I think you're likely heading in the right direction, though remember that we have to remain compliant with style rule prescriptions. Taking a quick glance through the tests I believe what you have so far fails to meet the requirements for the presence of any multi-lined element to require all subsequent elements to be on their own line individually https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/expressions.md#multi-line-elements |
Ahh I didn't see that part of the style guide. Thanks for pointing that out 😄. Fingers crossed making the change is a simple enough fix! I know you're a little more familiar with the |
Actually I may have been doing too much of a drive by and was mistaken. I think the chain layout in your target tests look fine as far as the chain pieces go (suspect I was looking at the source files thinking I was looking at the target), though do want to double check to see if there's potential for any unintended side effects on the arg front |
Definitely! Take the time you need to properly review the change and if you determine that there's something I'm overlooking here I'd be happy to tweak the implementation. |
src/chains.rs
Outdated
.collect::<Option<Vec<_>>>()?; | ||
|
||
format!("::<{}>", type_list.join(", ")) | ||
// subtract `{calle_str}::<>()` from the shape. We can only place everything on a single |
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.
Random passerby here, saw a typo:
// subtract `{calle_str}::<>()` from the shape. We can only place everything on a single | |
// subtract `{callee_str}::<>()` from the shape. We can only place everything on a single |
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.
I appreciate you calling it out!
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.
I appreciate you working on a tool I literally use hundreds of times a day!!
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.
Thank you so much! It honestly means a lot to hear that ❤️
Fixes 5249 Previously, we always formatted generic arguments horizontally. Now, we attempt to format generic items horizontally, but when doing so would lead to issue like exceeding the `max_width` we format the generic arguments vertically.
Closes 3191 PR 5252 resolves 3191
Closes 5657 PR 5252 resolves 5657
Fixes #5249
Previously, we always formatted generic arguments horizontally.
Now, we attempt to format generic items horizontally, but when doing so
would lead to issue like exceeding the
max_width
we format the genericarguments vertically.
Edit: After doing some backlog triage I found that this would also fix the following:
Closes #3191
Closes #5657