Skip to content

Format more text literals as multi-line strings #1508

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

Merged
merged 2 commits into from
Nov 4, 2019

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Nov 2, 2019

This causes text literals to be formatted as multi-line strings whenever they contain at least one newline and at least one non-newline character. "Spacers" like "\n\n" continue be formatted as single-line strings. If the heuristic turns out to be too eager to choose a multi-line layout, we can refine it later.

This partially addresses #1496.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 2, 2019

Two tests currently trigger a prettyprinter bug (quchen/prettyprinter#91). We could work around that by changing pretty_ to use our usual 80-columns layout instead of Unbounded:

-- | Pretty-print a value
pretty_ :: Pretty a => a -> Text
pretty_ = Pretty.renderStrict . Pretty.layoutPretty options . Pretty.pretty
where
options = Pretty.LayoutOptions { Pretty.layoutPageWidth = Pretty.Unbounded }

We also use Unbounded layouts in prettyToString, docToStrictText, and the diff tests:

let options =
Pretty.LayoutOptions
{ Pretty.layoutPageWidth = Pretty.Unbounded }
let actualDiffText =
Pretty.Text.renderStrict
(Pretty.layoutPretty options actualDiffDocument)

@Gabriella439
Copy link
Collaborator

@sjakobi: As discussed in the other issue, another way you can fix this is to ensure that every use of Line/hardline has a newline-free alternative provided via a flatAlt

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 3, 2019

another way you can fix this is to ensure that every use of Line/hardline has a newline-free alternative provided via a flatAlt

It seems to me that we couldn't then enforce the multi-line formatting of the text literals that this PR is about.

If it is important that pretty_ introduces as few line breaks as possible, we could instead use something like AvailablePerLine maxBound 1.0 for the LayoutOptions.

These are the problematic tests:

1

issue126 :: TestTree
issue126 = Test.Tasty.HUnit.testCase "Issue #126" (do
e <- Util.code "''\nfoo\nbar\n''"
Util.normalize' e @?= "\"foo\\nbar\\n\"" )

Here we use pretty_ via Core.pretty via normalize':

normalize' :: Expr Src Void -> Text
normalize' = Dhall.Core.pretty . Dhall.Core.normalize

2

_Interpolation_1 :: TestTree
_Interpolation_1 = Test.Tasty.HUnit.testCase "Example #1" (do
e <- Util.code
"''\n\
\ for file in *; do \n\
\ echo \"Found ''${file}\"\n\
\ done \n\
\'' \n"
Util.assertNormalized e )

We again use normalize' via assertNormalized.


I think my preference would be to simply use the 80-column layout consistently, so we don't have to worry about different layout variants.

@Gabriella439
Copy link
Collaborator

@sjakobi: I agree that the best solution here is to consistently avoid the Unbounded layout option, because it appears that it is incompatible with output that has obligate newlines

@sjakobi sjakobi changed the title WIP: Format more text literals as multi-line strings Format more text literals as multi-line strings Nov 3, 2019
This causes text literals to be formatted as multi-line strings
whenever they contain at least one newline and at least one non-newline
character. "Spacers" like `"\n\n"` continue be formatted as single-line
strings. If the heuristic turns out to be too eager to choose a
multi-line layout, we can refine it later.

This partially addresses #1496.

Also

* update some variable names

* use 80-column "smart" layout consistently
@sjakobi sjakobi force-pushed the sjakobi/1496-pretty-multi-line branch from 3485109 to 2f48830 Compare November 4, 2019 01:11
@mergify mergify bot merged commit e931451 into master Nov 4, 2019
@mergify mergify bot deleted the sjakobi/1496-pretty-multi-line branch November 4, 2019 03:32
sjakobi added a commit to dhall-lang/dhall-lang that referenced this pull request Nov 14, 2019
This changes the formatting of Dhall source files affected
by dhall-lang/dhall-haskell#183
or dhall-lang/dhall-haskell#1400.

The change in Prelude/Text/show is due to
dhall-lang/dhall-haskell#1508.
sjakobi added a commit to dhall-lang/dhall-lang that referenced this pull request Nov 17, 2019
This changes the formatting of Dhall source files affected
by dhall-lang/dhall-haskell#183
or dhall-lang/dhall-haskell#1400.

The change in Prelude/Text/show is due to
dhall-lang/dhall-haskell#1508, but also relies the bug fixes
dhall-lang/dhall-haskell/pull/1543 and
dhall-lang/dhall-haskell/pull/1550.
sjakobi added a commit to dhall-lang/dhall-lang that referenced this pull request Nov 23, 2019
This changes the formatting of Dhall source files affected
by dhall-lang/dhall-haskell#183
or dhall-lang/dhall-haskell#1400.

The change in Prelude/Text/show is due to
dhall-lang/dhall-haskell#1508, but also relies the bug fixes
dhall-lang/dhall-haskell/pull/1543 and
dhall-lang/dhall-haskell/pull/1550.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants