This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 660
fix(rome_formatter): Printer fill fits #3307
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for rometools canceled.
|
MichaReiser
commented
Sep 30, 2022
@@ -21,13 +21,13 @@ impl FormatNodeRule<JsxClosingElement> for FormatJsxClosingElement { | |||
|
|||
write![ | |||
formatter, | |||
[group(&format_args![ | |||
[ |
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.
These two changes aren't related to the bug but align our IR with Prettiers and made it easier to debug the issue
@denbezrukov this should fix the failing test in #3251 . @ematipico please go ahead with merging this PR and then #3251 on Monday (I'll check in on Saturday and merge the changes myself if they are approved but otherwise have to delegate, I'm sorry) |
!bench_formatter |
Formatter Benchmark Results
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
This fixes the issue why the test is currently failing in #3251
The specific snipped triggering the issue is:
That generates the following IR
The issue is the combination of
fill
withbest_fitting
.ENddddDSIIIN
fits on the line -> yes. Print the entry<div>text...</div>
fits on the line -> yes. Print the entry<div>text...</div> HRS
. Notice how it includes theHRS
which is from the nextfill
entry.The measuring if the best fitting should stop at the next separator because
fill
will make that separator expand if it otherwise risks exceeding the line width. Now, stopping at the next separator isn't trivial. However, there's no need forbest_fitting
to even test if it fits becausefill
already tested that.That's why the fix is somewhat simple. All that is necessary is to set that we know it will fit and that the printer can simply use the first variant.
But what about...
if the fill element doesn't fit. Do we not have the same problem then? Not really, because the element gets printed in expanded mode and this also applies for the separator coming right after. Since the separator must contain a soft line break the printer either decides that the content fits (for best fitting) or expands (group).
Test Plan
Tested that #3251 now formats correctly