Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Mar 3, 2022

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.


Edit: After doing some backlog triage I found that this would also fix the following:

Closes #3191
Closes #5657

@calebcartwright
Copy link
Member

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

@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 12, 2022

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 chain formatting, so I'm wondering if you could point me to where (or explain how) chains know to format all following values vertically?

@calebcartwright
Copy link
Member

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

@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 16, 2022

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

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:

Suggested change
// 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

Copy link
Contributor Author

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!

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!!

Copy link
Contributor Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants