Fix unstable f-string formatting for expressions containing a trailing comma #15545
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #15536
The
RemoveSoftlinesBuffer
removes soft line breaks andif_group_breaks
elements but itdoesn't remove
expand_parent
elements. This can lead to instable formattingwhen f-strings are wrapped in a
group
because it makes that group expand. For example,print(f"{ {}, 1, }")
first gets formatted toand only then converges at
print(f"{ {}, 1 }")
.I first tried to also remove
expand_parent
elemnents in theRemoveSoftLinesBuffer
but that results in many failing tests around assignment formatting because
we rely on
Document::will_break
to determine if we should try the flatlayout and that only returns
true
if the f-string formatting contains theexpand_parent
along the trailing comma.I reviewed all
expand_parent
usages and realized that the trailing comma is the onlyusage in our expression formatting. That's why I decided to adjust the trailing comma
formatting instead to omit the trailing comma if we know that we're in a flat f-string.
This is also how Dhruv implemented it originally before I removed the check in #14489 because I thought it
it no longer necessary.
Test Plan
Added regression test