Skip to content
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

Open
wants to merge 57 commits into
base: master
Choose a base branch
from
Open

Conversation

robrix
Copy link

@robrix robrix commented Aug 23, 2017

Per #39, this PR defines Pretty1 & Pretty2 classes, lifting Pretty to * -> * & * -> * -> * respectively, in the same manner as Show1 & Show2.

I’ve also defined a few instances of these new classes, as well as an instance of Pretty for Either a b, since it seemed incomplete to provide a Pretty2 instance for (,) but not for Either, and thereafter seemed incomplete to provide only lifted instances for Either.

I’ve confirmed that the tests (including doctests) all pass, but haven’t added any new tests since the instances are quite minimal.

Copy link
Author

@robrix robrix left a 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
Copy link
Author

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)
Copy link
Author

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.

Copy link
Owner

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)
Copy link
Owner

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. :-)
Copy link
Owner

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 [].
Copy link
Owner

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.

Copy link
Author

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.

Copy link
Owner

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! :-)

@robrix
Copy link
Author

robrix commented Aug 26, 2017

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?«

I quite like this idea! I think the [] instance isn’t gonna cut it, because it’s almost identical to the Pretty instance. Unfortunately, I’m having difficulty coming up with a better example. My own motivating examples tend to involve GADTs, signature functors, and/or functor composition, e.g.:

{-# 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?

Actually, this brings up a new law: Pretty1 f and Pretty (f a) should result in identical behavior! :-)

I suppose that if we were willing to compromise on that by accepting strange behaviour with String:

>>> liftPretty pretty "hello"
[h, e, l, l, o]

then we’d be able to pare things down significantly. I am having trouble imagining a situation where you’d use liftPretty on a String, to be honest; but I’ll defer to your preference.

@quchen
Copy link
Owner

quchen commented Oct 3, 2017

Back from summer hiatus! Sorry for the late reply. I don’t think String is important enough to break a leg over it to be honest! A short remark about this misalignment would be a good idea anyway, since for example Maybe a has a special case for lists as well.

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 -- ==== <title> (example)! Putting the long and somewhat complicated example in there would be fine: it allows interested parties to see them, while not being interrupting for the casual reader.

@robrix
Copy link
Author

robrix commented Oct 11, 2017

Back from summer hiatus! Sorry for the late reply.

No apologies necessary; hope you had a lovely break!

I don’t think String is important enough to break a leg over it to be honest! A short remark about this misalignment would be a good idea anyway, since for example Maybe a has a special case for lists as well.

Done and done 👍

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 -- ==== <title> (example)!

Oh, very cool! Thanks for sharing that gem 😊

Putting the long and somewhat complicated example in there would be fine: it allows interested parties to see them, while not being interrupting for the casual reader.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants