jyp / prettiest

The Prettiest Printer
GNU General Public License v3.0
35 stars 6 forks source link

Weird Aeson.Value layout #9

Closed phadej closed 7 years ago

phadej commented 7 years ago

Given an example (based on benchmark's prettiestValue):

{-# LANGUAGE OverloadedStrings #-}
module Main where

import Data.Aeson
import Text.PrettyPrint.Compact
import Data.Foldable (toList)

import qualified Data.HashMap.Lazy as H

prettiestJSON :: Value -> Doc ()
prettiestJSON (Bool True)  = text "true"
prettiestJSON (Bool False) = text "false"
prettiestJSON (Object o)   = encloseSep (text "{") (text "}") (text ",") (map prettyKV $ H.toList o)
  where prettyKV (k,v)     = text (show k) <> text ":" <+> prettiestJSON v
prettiestJSON (String s)   = string (show s)
prettiestJSON (Array a)    = encloseSep (text "[") (text "]") (text ",") (map prettiestJSON $ toList a)
prettiestJSON Null         = text "null"
prettiestJSON (Number n)   = text (show n)

value :: Value
value = object [ "foobar" .= three ] where
    three = object
        [ "1st" .= x
        , "2nd" .= x
        , "3rd" .= x
        ]
    x = object
        [ "kikka" .= Number 1
        , "kukka" .= Number 2
        , "pekka" .= Number 3
        , "matti" .= Number 4
        , "teppo" .= Number 5
        , "localization" .= Number 10
--        , "internationalization" .= Number 18
        ]

With some page widths, one gets weird looking results:

*Main> putStrLn $ renderWith Text.PrettyPrint.Compact.defaultOptions { optsPageWidth = 100 } $ prettiestJSON value
{"foobar": {"1st": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}, "3rd": {"matti": 4.0
                                          ,"pekka": 3.0
                                          ,"kukka": 2.0
                                          ,"localization": 10.0
                                          ,"kikka": 1.0
                                          ,"teppo": 5.0}, "2nd": {"matti": 4.0
                                                                 ,"pekka": 3.0
                                                                 ,"kukka": 2.0
                                                                 ,"localization": 10.0
                                                                 ,"kikka": 1.0
                                                                 ,"teppo": 5.0}}}

With slightly narrower (80) page, the layout is sensible :

{"foobar": {"1st": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}
           ,"3rd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}
           ,"2nd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}}}

and with wider (120) one pretty-compact does what I'd expect it to do:

{"foobar": {"1st": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"3rd": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"2nd": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}}}

I'm ok if this is a feature (I don't remember if you mention about such pathological cases in the paper), yet unfortunate one.

jyp commented 7 years ago

The horizontal concatenation <> does what you observe, namely tabbing the second argument against the last line of the first. This spec is necessary in certain cases. But I most certainly should add an operator which fails if its argument is multiline. On its basis we could make sure that horizontal or vertical concatenation works as you desire.

jyp commented 7 years ago

@phadej Please check the above commit to see if it solves the issue.

phadej commented 7 years ago

@jyp, it doesn't, I guess you have to actually implement

TODO: add a boolean to filter out multiline layouts.

for Doc :(

jyp commented 7 years ago

That's just an optimization.

I've fixed a couple of errors and it should be fine now.

Can you give it another try @phadej ?

(Branch https://github.com/jyp/prettiest/tree/single-line-cat)

phadej commented 7 years ago

@jyp it seems to work, but there's probably something slightly wrong:

width = 110

*Main> putStrLn $ renderWith Text.PrettyPrint.Compact.defaultOptions { optsPageWidth = 110 } $ prettiestJSON value
{"foobar": {"1st": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}
           ,"3rd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}
           ,"2nd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}}}

width = 111

Makes sense, cannot fit the two closing } for the last entry

*Main> putStrLn $ renderWith Text.PrettyPrint.Compact.defaultOptions { optsPageWidth = 111 } $ prettiestJSON value
{"foobar": {"1st": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"3rd": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"2nd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}}}

width = 112

Same as with 110 !!!, But should be the same as above?

*Main> putStrLn $ renderWith Text.PrettyPrint.Compact.defaultOptions { optsPageWidth = 112 } $ prettiestJSON value
{"foobar": {"1st": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}
           ,"3rd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}
           ,"2nd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}}}

width = 113

As expected

*Main> putStrLn $ renderWith Text.PrettyPrint.Compact.defaultOptions { optsPageWidth = 113 } $ prettiestJSON value
{"foobar": {"1st": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"3rd": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"2nd": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}}}
phadej commented 7 years ago

The 111 output is precisely the width to fit 1nd and 3rd:

*Main> putStrLn $ take 110 $ cycle "0123456789"
01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
*Main> putStrLn $ renderWith Text.PrettyPrint.Compact.defaultOptions { optsPageWidth = 111 } $ prettiestJSON value
{"foobar": {"1st": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"3rd": {"matti": 4.0, "pekka": 3.0, "kukka": 2.0, "localization": 10.0, "kikka": 1.0, "teppo": 5.0}
           ,"2nd": {"matti": 4.0
                   ,"pekka": 3.0
                   ,"kukka": 2.0
                   ,"localization": 10.0
                   ,"kikka": 1.0
                   ,"teppo": 5.0}}}
jyp commented 7 years ago

I fixed the bug.

I believe that the behaviour for "encloseSep" (and hcat) is that most will want, so I'm merging into the master.

phadej commented 7 years ago

I confirm, it seems to work properly for me. Thanks!