Open mlugg opened 4 years ago
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.
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.
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.
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= []
}
@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.
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.
@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
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?
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.
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 = []
}
I'm going to keep this open despite #110, seeing as the output for https://github.com/cdepillabout/pretty-simple/issues/84#issuecomment-693375305 is still odd:
(
( 1, 2 ), Result
{ a = True, b = False }
)
There's also https://github.com/cdepillabout/pretty-simple/issues/84#issuecomment-729292927, 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?
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.
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!
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 https://github.com/cdepillabout/pretty-simple/issues/9.
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...
I'm using a custom pPrintOpt config which looks like this:
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:
If I define a simple ADT, it also works fine:
However, if I try and print a list of tuples, the output seems to insert extra newlines after the commas:
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.