Skip to content
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

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Fix Text wrapping #16277

merged 1 commit into from
Nov 10, 2022

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Nov 3, 2022

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

@dwijnand dwijnand force-pushed the fix-text-wrapping branch 2 times, most recently from 1355dbc to c497fae Compare November 3, 2022 17:02
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.
@dwijnand dwijnand marked this pull request as ready for review November 4, 2022 16:06
@dwijnand dwijnand requested a review from SethTisue November 4, 2022 16:06
@dwijnand
Copy link
Member Author

dwijnand commented Nov 7, 2022

This is easy to hit while printing tree structures, for example:

      DefDef(a1, List(List(ValDef(b, Ident(Bar), Thicket(List())))), Ident(Foo)
        ,
      Block(List(

Copy link
Member

@SethTisue SethTisue left a 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.

@dwijnand dwijnand requested a review from lrytz November 10, 2022 11:28
@dwijnand dwijnand assigned lrytz and unassigned SethTisue Nov 10, 2022
@dwijnand dwijnand merged commit d876acf into scala:main Nov 10, 2022
@dwijnand dwijnand deleted the fix-text-wrapping branch November 10, 2022 12:42
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants