Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Lifted Pretty classes - #40

Open
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes
Open

Lifted Pretty classes#40
robrix wants to merge 57 commits into
haskell-prettyprinter:masterfrom
robrix:lifted-pretty-classes

Conversation

@robrix

Copy link
Copy Markdown

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.

@robrixrobrix left a comment

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 #-}
dataExpra=Plusaa | Timesaa | ConstIntnewtypeFixf=Fix (f (Fixf))
dataFreeFfab=Free (fb) | PureatypeFreefa=Fix (FreeFfa)
instancePretty1Exprwhere
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
instancePretty1f=>Pretty2 (Freef) where
liftPretty2 pA _ pB plB = go
where go (Pure a) = pA a
go (Free f) = liftPretty pB plB f
instance (Pretty1f, Prettya) =>Pretty1 (Freef) where
liftPretty = liftPretty2 pretty prettyList
instancePretty1f=>Pretty (Fixf) where
pretty (Fix f) = liftPretty pretty prettyList f

vs.

{-# LANGUAGE UndecidableInstances #-}
instance (Pretty (fb), Prettya) =>Pretty (FreeFfab) where
pretty (Pure a) = pretty a
pretty (Free f) = pretty f
instancePretty (f (Fixf)) =>Pretty (Fixf) 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

quchen commented Oct 3, 2017

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Author

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 freeto 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

@robrix@quchen