-
Notifications
You must be signed in to change notification settings - Fork 36
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
Lifted Pretty classes #40
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for review 🙏
:: (a -> Doc ann) -- ^ A function to print a single value. | ||
-> ([a] -> Doc ann) -- ^ A function to print a list. Used for []. | ||
-> f a | ||
-> Doc ann |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that there’s nothing gained by providing liftPrettyList
; the list-printing parameter suffices to pretty-print [Char]
correctly using liftPretty
:
λ: liftPretty pretty prettyList "hello"
hello
liftPrettyList
& liftPrettyList2
can make some situations involving nested Pretty1
instances a little more convenient, but as they’re essentially always given the default definitions (as follows), I have omitted them to keep surface area down.
liftPrettyList pretty' prettyList' = list . map (liftPretty pretty' prettyList')
-- >>> liftPretty2 (parens . pretty) (list . map (parens . pretty)) (parens . pretty) (list . map (parens . pretty)) (Left True :: Either Bool Bool) | ||
-- (True) | ||
-- >>> liftPretty2 (parens . pretty) (list . map (parens . pretty)) (parens . pretty) (list . map (parens . pretty)) (Right True :: Either Bool Bool) | ||
-- (True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These doctests are unfortunately quite lengthy, but it’s a little difficult providing good concise examples for nonrecursive types.
Nevertheless, I’ve found these instances to be quite useful in practice, so I felt it was worth providing them, lengthy doctests and all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests would be nicer if they weren’t on a single line, but with let
helper definitions with good names.
-- >>> liftPretty2 (parens . pretty) (list . map (parens . pretty)) (parens . pretty) (list . map (parens . pretty)) (Left True :: Either Bool Bool) | ||
-- (True) | ||
-- >>> liftPretty2 (parens . pretty) (list . map (parens . pretty)) (parens . pretty) (list . map (parens . pretty)) (Right True :: Either Bool Bool) | ||
-- (True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests would be nicer if they weren’t on a single line, but with let
helper definitions with good names.
-- | ||
-- Laws: | ||
-- | ||
-- 1. output should be pretty. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be an example for an instance definition here, for example the []
one. And the law joke doesn’t need to be repeated again ;-)
I think the key to the Pretty1 documentation should be showing how it is useful without going into too much detail. Like »here we define Pretty without using Pretty1, see how Pretty1 makes our lives much easier?«
-- (hello) | ||
liftPretty | ||
:: (a -> Doc ann) -- ^ A function to print a single value. | ||
-> ([a] -> Doc ann) -- ^ A function to print a list. Used for []. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary? It clutters the definitions quite a bit, but it’s usually just list . map pretty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s necessary for the []
definition to work as expected for Char
, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrr, I see.
Actually, this brings up a new law: Pretty1 f
and Pretty (f a)
should result in identical behavior! :-)
I quite like this idea! I think the {-# LANGUAGE GADTs #-}
data Expr a = Plus a a | Times a a | Const Int
newtype Fix f = Fix (f (Fix f))
data FreeF f a b = Free (f b) | Pure a
type Free f a = Fix (FreeF f a)
instance Pretty1 Expr where
liftPretty p _ (Plus a b) = parens $ p a <+> pretty '+' <+> p b
liftPretty p _ (Times a b) = p a <+> pretty '*' <+> p b
liftPretty _ _ (Const i) = pretty i
instance Pretty1 f => Pretty2 (Free f) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1 f, Pretty a) => Pretty1 (Free f) where
liftPretty = liftPretty2 pretty prettyList
instance Pretty1 f => Pretty (Fix f) where
pretty (Fix f) = liftPretty pretty prettyList f vs. {-# LANGUAGE UndecidableInstances #-}
instance (Pretty (f b), Pretty a) => Pretty (FreeF f a b) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instance Pretty (f (Fix f)) => Pretty (Fix f) where
pretty (Fix f) = pretty f This seems a little overboard for the docs, but I’m not sure what would be a better example 😕 What do you think?
I suppose that if we were willing to compromise on that by accepting strange behaviour with
then we’d be able to pare things down significantly. I am having trouble imagining a situation where you’d use |
Back from summer hiatus! Sorry for the late reply. I don’t think Your documentation looks good, but it’s too verbose to show it in the default view. But good news, I recently found out that Haddock allows collapsible sections via |
No apologies necessary; hope you had a lovely break!
Done and done 👍
Oh, very cool! Thanks for sharing that gem 😊
Excellent. It took me a bit to figure out how to make doctest accept multi-line input for the instances, but I’ve now added that and it’s looking good 👍 I appreciate you taking the time to workshop this PR with me. IMO the code has benefitted greatly from it! Let me know what you think of the latest revisions. |
Per #39, this PR defines
Pretty1
&Pretty2
classes, liftingPretty
to* -> *
&* -> * -> *
respectively, in the same manner asShow1
&Show2
.I’ve also defined a few instances of these new classes, as well as an instance of
Pretty
forEither a b
, since it seemed incomplete to provide aPretty2
instance for(,)
but not forEither
, and thereafter seemed incomplete to provide only lifted instances forEither
.I’ve confirmed that the tests (including doctests) all pass, but haven’t added any new tests since the instances are quite minimal.