-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix Text wrapping #16277
Fix Text wrapping #16277
Conversation
1355dbc
to
c497fae
Compare
There was a problem with how some text wrapped. I noticed it in the printing of big tuple types and I started to write unit tests for those, but there turned out to also be a checkfile that demonstrates the fix: tests/neg/i14127.check: - | Int - |, Int, Int)] was found for parameter x [...] - | Int - |, Int, Int)]: + | Int, Int, Int)] was found for parameter [...] + | Int, Int, Int)]: The fix is to make appendIndented, which is used by append, use Fluid instead of switching to Vertical. It seems to me that, pre-layout, Vertical means "I want my elements to be vertical", i.e. wrap every one, and it's layout that takes that into account. Fluid instead means "wrap where necessary, pack where possible". Post-layout, instead, it looks like Fluid means one-per-line, as it's consumed by print. So the fix is to switch appendIndented from Vertical to Fluid. Following that I made some improvements like not needless boxing non-splittable text, like Str, into Closed and merging non-closed Fluid text during concat (`~`). That fixed the extra wrapping, but it meant that the ", " separator could be the text that is wrapped and indented, which didn't look nice. So I changed Text.apply to close the separator to the previous element. That encouraged lines ending with ", ", that is with a trailing space, so I made print strip trailing spaces. The change from Vertical to Fluid made the last parenthesis in tests/neg/i9185.check not wrap, which is fine, but that pulled the trailing "failed with" up to the previous line. As that's not code, I decided to move it down and a line away from the code.
c497fae
to
9852984
Compare
This is easy to hit while printing tree structures, for example:
|
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.
This looks good, but I'm a hesitant to hit “merge” myself since I don't know if anyone considers themselves the owner (or, at least, a knowledgeable reviewer) of the pretty-printing code, which I don't have that much understanding of.
Regardless, there's a good case for just optimistically merging this, since it definitely fixes some pretty egregiously bad formatting, and the checkfile updates for the existing tests that were affected LGTM.
There was a problem with how some text wrapped. I noticed it in the
printing of big tuple types and I started to write unit tests for those,
but there turned out to also be a checkfile that demonstrates the fix:
tests/neg/i14127.check:
The fix is to make appendIndented, which is used by append, use Fluid
instead of switching to Vertical. It seems to me that, pre-layout,
Vertical means "I want my elements to be vertical", i.e. wrap every one,
and it's layout that takes that into account. Fluid instead means "wrap
where necessary, pack where possible". Post-layout, instead, it looks
like Fluid means one-per-line, as it's consumed by print. So the fix is
to switch appendIndented from Vertical to Fluid.
Following that I made some improvements like not needless boxing Str
into Closed and merging non-closed Fluid text during concat (
~
).Also, I changed close to preserve the Vertical wrapping, but didn't
investigate that more.
That fixed the extra wrapping, but it meant that the ", " separator
could be the text that is wrapped and indented, which didn't look nice.
So I changed Text.apply to close the separator to the previous element.
That encouraged lines ending with ", ", that is with a trailing space,
so I made print strip trailing spaces.
The change from Vertical to Fluid made the last parenthesis in
tests/neg/i9185.check not wrap, which is fine, but that pulled the
trailing "failed with" up to the previous line. As that's not code, I
decided to move it down and a line away from the code.