"Compact" option sometimes produces ugly/inconsistent output

[mlugg]

I'm using a custom pPrintOpt config which looks like this:

pPrint :: (Show a) => a -> IO ()
pPrint = pPrintOpt CheckColorTty (defaultOutputOptionsDarkBg {outputOptionsCompact=True,outputOptionsPageWidth=60})

I'm getting some weird behaviour when printing long lists of tuples with this compact option. If I print a list of 10 integer values, the printing works as expected:

Prelude> replicate 10 12345
[ 12345
, 12345
, 12345
, 12345
, 12345
, 12345
, 12345
, 12345
, 12345
, 12345
]

If I define a simple ADT, it also works fine:

Prelude> data Foo = Foo Int Int deriving (Show)
Prelude> replicate 10 (Foo 1 2)
[ Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
, Foo 1 2
]

However, if I try and print a list of tuples, the output seems to insert extra newlines after the commas:

Prelude> replicate 10 (1, 2)
[
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
,
    ( 1, 2 )
]

I would not expect these newlines to be here. Instead, the expected output would be similar to the previous examples, where the comma is on the same line as the value.

[georgefst]

Yep, agreed that's unfortunate. Adding outputOptionsCompact was essentially a quick one line change, so there are bound to be some awkward cases we haven't thought of.

I'll try to look in to this, but it's unlikely to be particularly high-priority for me in the near future. If you're willing to investigate yourself, the key function is prettyExpr. It's admittedly a bit dense, but you'll see that compactness currently just means applying group to each subexpression. Ideally, I'd like to keep it that way, so we'd want to find a way to use combinators like line, line' and flatAlt effectively. Also note that the behaviour of group can be bit odd.

[georgefst]

Well, what do you know, I've actually just hit this myself, and it is a little annoying. So that's a slight bump in priority.

[georgefst]

For posterity, mine is:

x :: IO ()
x = pPrintOpt CheckColorTty opts
    (
        ( 1, 2 )
    , Result
        { a = True, b = False }
    )
  where
    opts =
        defaultOutputOptionsDarkBg
            { outputOptionsCompact = True
            , outputOptionsPageWidth = 40
            }

Ideally, I'd like the constructor (Result) on the same line as well.

[simonmichael]

Thanks for pretty-simple ! hledger has switched to it. Possibly related to this issue: if it could produce slightly more compact output the way pretty-show did, that would be even better. Eg:

pretty-show:

   CsvRules
     { rdirectives = [ ( "skip" , "1" ) ]
     , rcsvfieldindexes = [ ( "date" , 1 ) , ( "amount" , 2 ) ]
     , rassignments = [ ( "amount" , "%2" ) , ( "date" , "%1" ) ]
     , rconditionalblocks = []
     }

pretty-simple, compact mode:

   CsvRules
     { rdirectives=
       [ ( "skip", "1" ) ]
     , rcsvfieldindexes=
       [ ( "date", 1 ), ( "amount", 2 ) ]
     , rassignments=
       [ ( "amount", "%2" ), ( "date", "%1" ) ]
     , rconditionalblocks= []
     }

[georgefst]

@simonmichael Yeah, I agree that would be preferable. As I mentioned further up the thread, the compact mode is really a one line hack, so there hasn't been any thought put in to these edge cases. I'll have a think about how easy this particular one would be to fix.

[georgefst]

CsvRules
{ rdirectives=
[ ( "skip", "1" ) ]
, rcsvfieldindexes=
[ ( "date", 1 ), ( "amount", 2 ) ]
, rassignments=
[ ( "amount", "%2" ), ( "date", "%1" ) ]
, rconditionalblocks= []
}

I assume you're doing something other than calling pPrintOpt on your data directly? Otherwise the lack of space before = is another, separate, bug.

[simonmichael]

@georgefst no I'm just doing:

prettyopts = 
    defaultOutputOptionsDarkBg
    -- defaultOutputOptionsLightBg
    -- defaultOutputOptionsNoColor
    { outputOptionsIndentAmount=2
    , outputOptionsCompact=True
    }

-- | Pretty print. Generic alias for pretty-simple's pPrint.
pprint :: Show a => a -> IO ()
pprint = pPrintOpt CheckColorTty prettyopts

[georgefst]

Okay, with that and:

data CsvRules = CsvRules
    { rdirectives :: [(String, String)]
    , rcsvfieldindexes :: [(String, Int)]
    , rassignments :: [(String, String)]
    , rconditionalblocks :: [()]
    }
    deriving (Show)

main = pprint CsvRules
    { rdirectives = [("skip", "1")]
    , rcsvfieldindexes = [("date", 1), ("amount", 2)]
    , rassignments = [("amount", "%2"), ("date", "%1")]
    , rconditionalblocks = []
    }

I'm getting:

CsvRules
  { rdirectives =
    [ ( "skip", "1" ) ]
  , rcsvfieldindexes =
    [ ( "date", 1 ), ( "amount", 2 ) ]
  , rassignments =
    [ ( "amount", "%2" ), ( "date", "%1" ) ]
  , rconditionalblocks = []
  }

Do you not? Does CsvRules have a custom Show instance?

[simonmichael]

Aha, yes it does: https://github.com/simonmichael/hledger/blob/master/hledger-lib/Hledger/Read/CsvReader.hs#L267 . That's our bug, thanks for looking into it.

[simonmichael]

PS, just more brainstorming: aligning equals signs (capped at some not-too-large width) might be nice for readability:

CsvRules
  { rdirectives        = [ ( "skip" , "1" ) ]
  , rcsvfieldindexes   = [ ( "date" , 1 ) , ( "amount" , 2 ) ]
  , rassignments       = [ ( "amount" , "%2" ) , ( "date" , "%1" ) ]
  , ralignmentisrelaxedforthingswiderthanthelimit = []
  , rconditionalblocks = []
  }

[georgefst]

I'm going to keep this open despite #110, seeing as the output for #84 (comment) is still odd:

(
    ( 1, 2 ), Result
    { a = True, b = False }
)

There's also #84 (comment), though that's unrelated to the main thrust of this issue, which is about making compact mode more compact. Perhaps, @simonmichael, you might open a separate issue if you're still interested?

[juhp]

I spent a little time looking at this yesterday actually,
and came to the conclusion that the simplest way to fix this is to make Expr a recursive type
so it can hold any Haskell value, otherwise the context is lost for data constructors.

[georgefst]

the simplest way to fix this is to make Expr a recursive type so it can hold any Haskell value, otherwise the context is lost for data constructors

Yes, that sounds reasonable!

[cdepillabout]

One thing to possibly watch out for is to make sure not to remove the ability to pretty-print an infinite data structure. Currently the parsing and pretty-printing in pretty-simple is mostly lazy, so you can do something like:

> pPrint (repeat [(Just [1,2,3], ("hello", "bye"))])
[
    [
        ( Just
            [ 1
            , 2
            , 3
            ]
        ,
            ( "hello"
            , "bye"
            )
        )
    ]
,
    [
        ( Just
            [ 1
            , 2
            , 3
            ]
...
^C
>

Depending on how you're thinking of implementing this, you'll may want to watch out that you don't accidentally remove this lazy parsing / pretty-printing ability.

This was added in #9.

[cdepillabout]

Oh, and I realized that currently pretty-simple doesn't parse and pretty-print lazily with the compact option:

> pPrintOpt CheckColorTty defaultOutputOptionsDarkBg { outputOptionsCompact = True }  (repeat 3)

This doesn't print anything. Maybe I should make a separate issue about this...