Skip to content

Changes to enable usage of prettyprinter backends#73

Merged
georgefst merged 3 commits intocdepillabout:masterfrom
georgefst:expose-pp-internals
Aug 18, 2020
Merged

Changes to enable usage of prettyprinter backends#73
georgefst merged 3 commits intocdepillabout:masterfrom
georgefst:expose-pp-internals

Conversation

@georgefst
Copy link
Copy Markdown
Collaborator

Using prettyprinter-ansi-terminal's AnsiStyle to represent colours was not ideal as it is intentionally opaque, and so it's difficult to convert to another format.

This PR introduces a new Style type corresponding to the subset of AnsiStyle which is used in pretty-simple.

There is some inconsistency in whether we refer to things as "style"s or "colour"s. This Style type allows customising whether the text is bold, so it does represent more than just colour. But changing all instances of "color" to "style" would seem an unnecessary break of backwards compatibility. This isn't really a new issue, but this change has made it more obvious.

This makes it feasible to use pretty-simple with any prettyprinter backend.
-- Used by 'Simple.pString' etc.
layoutString :: OutputOptions -> String -> SimpleDocStream AnsiStyle
layoutString :: OutputOptions -> String -> SimpleDocStream Style
layoutString opts =
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should actually document this functionality properly, and expose this function from a non-Internal module?

colorDullMagenta = colorDull Magenta

colorDullRed :: AnsiStyle
colorDullRed = colorDull Red
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have gone overboard in removing things from this module. But with this more user-friendly colour type (previously it was just Builder), it seems a bit pointless to export all these combinations.

import Prettyprinter (SimpleDocStream)
import Prettyprinter.Render.Terminal
(Color (..), Intensity(Vivid,Dull), AnsiStyle,
renderLazy, renderIO)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't quite been able to work out the formatting rules for imports. Do you use a particular formatter?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hindent used to format imports like this (but now it has been changed to format import lists differently).

However, if you want to change this (or even run a formatter over all the code), please feel free.

@georgefst
Copy link
Copy Markdown
Collaborator Author

There are a few missing Haddocks. I'll add those once everything else is approved.

@cdepillabout
Copy link
Copy Markdown
Owner

@georgefst You've been doing a bunch of work on pretty-simple lately (including various dependencies of pretty-simple).

Would you be interested in become a maintainer of pretty-simple? That way, you could review PRs, as well as merge your own PRs and make releases without having to wait for me.

I've been pretty busy at work lately, and I haven't had time to take a good look at your last couple PRs.

@georgefst
Copy link
Copy Markdown
Collaborator Author

Would you be interested in become a maintainer of pretty-simple?

That would be great. If you're happy not to review the current PRs in detail, then I'll clean them up and merge. I'd still feel more comfortable checking in with you for at least a high-level overview of changes before publishing a release (and I really don't mind waiting a while for that if necessary).

@cdepillabout
Copy link
Copy Markdown
Owner

@georgefst I sent you an invitation to be a collaborator for this repo. I think you should get an email from github about it.

Also, if you send me your hackage username, I'll add you as a maintainer on Hackage.

I'd still feel more comfortable checking in with you for at least a high-level overview of changes before publishing a release

I'd definitely be fine with checking for a high-level overview of changes, although you can feel free to go ahead and merge stuff yourself, as well as review/merge PRs from other users.

For making a release to Hackage, I have a checklist: https://functor.tokyo/blog/2018-07-16-release-haskell-packages-to-hackage

Most of these should be pretty obvious, but the two important points are to make sure to update the CHANGELOG.md before doing a release, and then to make sure to create a tag and push it to GitHub on the commit that you uploaded to Hackage.


As far as the long-term vision for pretty-simple, the two points that are the most important to me are the following:

  • pretty-simple should be simple to use. Most users should just be able to import pPrint and use it.
  • pretty-simple should try to hard prettify and print anything you throw at it (even if it is not valid Haskell code). The output doesn't have to perfect but it should at least be better than Haskell's normal print output.

@cdepillabout
Copy link
Copy Markdown
Owner

I took a quick look at this, and I'm happy with it if you want to merge it in!

@georgefst
Copy link
Copy Markdown
Collaborator Author

@cdepillabout I'm GeorgeThomas on Hackage.

@cdepillabout
Copy link
Copy Markdown
Owner

Thanks, I've added you on Hackage: https://hackage.haskell.org/package/pretty-simple/maintainers/

@georgefst georgefst merged commit 4a5a2b7 into cdepillabout:master Aug 18, 2020
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