Fix unnecessary space around power op in overlong f-string expressions #14489
Conversation
MichaReiser
added
bug
MichaReiser
force-pushed
the
micha/remove-soft-line-conditional-content
branch
from
2bd63c3 to
719e57b
Compare
|
crates/ruff_formatter/src/buffer.rs
Outdated
| FormatElement::Tag(Tag::StartConditionalContent(condition)) | ||
| if condition.mode.is_expanded() => | ||
| { | ||
| self.state.increment_conditional_content(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is just for my understanding - the reason this exists here in addition to being in should_drop as well is because of the Default state which would mean that the should_drop will return false. But, we don't need to match against EndConditionalContent as it'll be taken care during the should_drop check.
I wonder if this could be simplified to only have a counter where any value > 0 indicates that we're in "if_group_breaks" and 0 indicates that we're outside of it. Or, do you foresee any other states that we might want to track in RemoveSoftLineBreaksState?
There was a problem hiding this comment.
We could probably just use one usize for it where 0 indicates that we're inside an if_group_breaks if we change the implementation to start the level at 1 instead of zero.
However, I do like the explicit state because it documents what's going on here.
There was a problem hiding this comment.
I'm fine with the explicit state, I just found the multiple usages of incrementing but only a single usage of decrementing the value a bit confusing. I had to look at it a bit closely to understand why this is the case.
There was a problem hiding this comment.
I just found the multiple usages of incrementing but only a single usage of decrementing the value a bit confusing
I don't think that would change by switching to using an usize instead of an explicit enum (we could "hide it" by using saturating_sub).
There was a problem hiding this comment.
There was actually a bug that we called increment twice in some cases. I refactored the code a bit
MichaReiser
force-pushed
the
micha/remove-soft-line-conditional-content
branch
from
1059392 to
844e0e4
Compare
… in expression part (#14811) ## Summary Fixes #14778 The formatter incorrectly removed the inner implicitly concatenated string for following single-line f-string: ```py f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}" # formatted f"{ if True else ''}" ``` This happened because I changed the `RemoveSoftlinesBuffer` in #14489 to remove any content wrapped in `if_group_breaks`. After all, it emulates an *all flat* layout. This works fine when `if_group_breaks` is only used to **add** content if the gorup breaks. It doesn't work if the same content is rendered differently depending on if the group fits using `if_group_breaks` and `if_groups_fits` because the enclosing `group` might still *break* if the entire content exceeds the line-length limit. This PR fixes this by unwrapping any `if_group_fits` content by removing the `if_group_fits` start and end tags. ## Test Plan added test
Summary
Fixes #14487
The
RemoveSoftLineBuffershould remove any content "expanded"-only content. For example, it removes all soft line breaks and replacessoft_line_break_or_spacewith aspace.However, it didn't remove any
if_group_breaksusages.This PR extends the
RemoveSoftLineBufferto also remove any content insideif_group_breaks. This is slightly more complicated because it requires removing multiple elements andif_group_breakscall can be nested.Test Plan
Added test