Better support for multiline literals

[sol]

No description provided.

[sol]

@mpilgrem this needs a review.

@mpilgrem @sellout this also needs some manual smoke testing on real packages before it can be released, to minimize the risk of any accidental breakage.

[sol]

Any help with that is very much appreciated.

[mpilgrem]

@sol, I read the code. I still need to try out the revised Hpack, locally, but you have added tests.

[mpilgrem]

I noticed the use of Bool here, rather than some sum datatype. In other circumstances, the code base has a number of sum datatypes that are isomorphic to Bool.

This lead me to ponder on:

renderSettingsEmptyLines :: EmptyLineStyle

data EmptyLineStyle
  = ELAsDot -- Legacy (Cabal file specification < 3.0)
  | ELVerbatim
  deriving (Eq, Show)

I appreciate it would make functions emptyLineToDot and settings below a little more complex.

It is a matter of subjective opinion whether that would make the code more expressive and, if it did, whether that gain would outweigh the additional complexity.

[sol]

Personally I am OK with using a Bool value for a record field, as the field name describes the semantic.

But at the point you pass things around as positional arguments it's probably time to reach for a custom data time.

Here specifically, RenderSettings is exposed through the API, but any user specified renderSettingsEmptyLinesAsDot is always overruled. So I think this should actually not be part of render settings, so maybe I should turn it into a positional argument and indeed use a custom data type.

[sol]

I ended up with f8e64c5. A positional argument turned out to get messy..

[mpilgrem]

I experimented with package.yaml:

# verbatim:
#   cabal-version: 3.0

spec-version: 0.36.0

name: testDotStyle
version: 0.1.0.0
description: |
  Paragraph One.

  Paragraph Two.
homepage: |
  Paragraph One.

  Paragraph Two.
bug-reports: |
  Paragraph One.

  Paragraph Two.
author: |
  Paragraph One.

  Paragraph Two.
maintainer: |
  Paragraph One.

  Paragraph Two.
copyright: |
  Paragraph One.

  Paragraph Two.
stability: |
  Paragraph One.

  Paragraph Two.
# We try starting the synopsis with an empty line:
synopsis: |

  Paragraph One.

  Paragraph Two.
# Note, from a Hackage perspective, this is a malformed use of catagory:
category: |
  Paragraph One.

  Paragraph Two.

flags:
  my-flag:
    description: |
      Paragraph One.

      Paragraph Two.
    manual: false
    default: false

dependencies:
- base >= 4.7 && < 5

executables:
  testDotStyle:
    source-dirs: app
    main: Main.hs
#   generated-other-modules:
#   - PackageInfo_testDotStyle

and executable testDotStyle:

module Main (main) where

import PackageInfo_testDotStyle ( synopsis )

main :: IO ()
main = putStrLn synopsis

and the Hpack output was as I expected it to be, including when cabal-version: 3.0.

[mpilgrem]

However, I realised that the legacy (<3.0) Cabal file format is capable of things that the modern format cannot handle, as follows:

With the legacy format, . is interpreted as an empty line and it is possible to specify a freeform value in a Cabal file that starts with an empty line (extract):

cabal-version: 1.12
synopsis:       .
                Paragraph One.
                .
                Paragraph Two.

With the modern format, is not not possible to specify a freeform value in a Cabal file that starts with an empty line. The following is not interpreted as starting with an empty line:

cabal-version: 3.0
synopsis:       
                Paragraph One.
                
                Paragraph Two.

With the modern format, . is interpreted literally. So, you cannot solve that problem with:

cabal-version: 3.0
synopsis:       .       
                Paragraph One.
                .
                Paragraph Two.

@sol, given that, should Hpack warn (or error) if it encounters, in a package.yaml file, a multi-line literal that starts with an empty line when cabal-version: is 3.0 or greater? EDIT: Or, perhaps, initial empty lines should be dropped when not renderSettingsEmptyLinesAsDot - something like:

renderValue :: RenderSettings -> Value -> Lines
renderValue RenderSettings{..} = \ case
  Literal string -> case lines string of
    [value] -> SingleLine value
    values -> MultipleLines Align $ if renderSettingsEmptyLinesAsDot
      then
        map emptyLineToDot values
      else
        -- The Cabal file specification >= 3.0 cannot handle initial empty lines:
        dropWhile isEmptyLine values
  WordList ws -> SingleLine $ unwords ws
  LineSeparatedList xs -> renderLineSeparatedList renderSettingsCommaStyle xs
  CommaSeparatedList xs -> renderCommaSeparatedList renderSettingsCommaStyle xs
  where
    emptyLineToDot :: String -> String
    emptyLineToDot xs
      | isEmptyLine xs = "."
      | otherwise = xs

    isEmptyLine :: String -> Bool
    isEmptyLine = all isSpace

[mpilgrem]

As discussed in the conversation above, the Cabal file specification >= 3.0 cannot handle initial empty lines, so perhaps just ignore them? Something like:

    values -> MultipleLines Align $ if renderSettingsEmptyLinesAsDot
      then
        map emptyLineToDot values
      else
        -- The Cabal file specification >= 3.0 cannot handle initial empty lines:
        dropWhile isEmptyLine values
  ...
  where
    emptyLineToDot :: String -> String
    emptyLineToDot xs
      | isEmptyLine xs = "."
      | otherwise = xs

[sol]

I'm not inclined to warn.

At first, I really liked your idea of stripping leading empty lines when specification >= 3.0. However, after trying a couple of things I realized that there are more differences in how Cabal interprets multi-line strings depending on the Cabal spec version. Apparently pre-3 strips leading white space, while post-3 keeps them intact (try e.g. synopsis: "\n foo\nbar\nbaz\n").

My current position is this:

• Cabal changes the semantics on how it interprets multi-line strings across spec versions. It did so in the past, and it may do so again in the future. Obviously, we need to make sure that we generate valid .cabal files this is why we need to replace empty lines with dots depending on the spec version, but other than that I think it's best to leave the interpretation of those strings to Cabal (at least in situations where it is not strictly clear that there is a better alternative), and then let the user and Cabal figure things out.
• Still, I think the good news is that leading empty lines are not commonly used and that most real world package descriptions will just work fine across different spec versions.

[sol]

@sol, I read the code.

Thanks a lot @mpilgrem.

you have added tests

My worry is not that the new functionality could have issues.

I'd be more worried if this would break any existing corner cases. I don't think it's likely, but the impact would be significant. That's why I wanted to run it on some existing specifications before releasing, just as an extra precaution (the changed code affects a lot of fields).