feat: Add pretty printer for expressions.

[patrickaldis]

Need to:

• Render types - currently only expressions are rendered
• Increase clarity via:
  • Reduce use of brackets (if possible)
  • Indent based on brackets

[georgefst]

The actual important Haskell code in prettyExpr and prettyType is great. And the output is indeed very pretty! Most of the comments above are fairly nitpicky.

I have a few more suggestions related to git. Depending on how comfortable you are with git rebasing, maybe we should save these for when we next meet in person. I'm also happy not to actually fix any of these, and just bear these points in mind as guidance for the future, as long as @dhess feels the same way.

• We try to follow the Conventional Commits spec (but we aren't particularly strict about it). The commit 10b6a70 is missing a fix:/feat:/refactor: prefix.
• It's not super important, but commit messages are usually written in the imperative present tense. So, for example, change "Implemented" to "Implement".
• The commit history is slightly messy in places. For example, some of the changes in 10b6a70 seem like they'd belong better in 56183b2.

Also CI is currently failing. The reasons can be seen by clicking on one of the red crosses, but to be fair, the output isn't always easy to decipher.

• The added test is failing. You can see this by running on your local machine: :main -p Pretty fails now that we print types properly. Try :main -p Pretty --accept and commit the new version.
• Weeder is complaining about the fact that three of the added functions have been removed.
  • We should definitely add prettyPrintExpr and prettyPrintType to the ignoreRoots list in weeder.dhall, since they fit the description in the comment there.
  • On the other hand, not should perhaps just be removed. It was a convenience for exploratory testing, but now that we have actual tests it doesn't serve much purpose. Alternatively, we could utilise not in a new test case in Tests.Pretty. There's something to be said for having a simpler, more legible test case in addition to testing the large (and nonsensical) comprehensive definition.
• The Cabal file is no longer formatted as we require. This is actually my bad, as I'm the one who added the dependency with a long name (prettyprinter-ansi-terminal). Run cabal-fmt -i primer/primer.cabal.
  • FWIW, I believe the next release of HLS will have support for cabal-fmt, so that we can just use format-on-save as we do for our .hs files.

[dhess]

Was there any attempt here to align the prettyprinter colors with our frontend tree color scheme? And if not, does it make sense to do so? (We could do it as a later PR — it's not particularly high priority.)

[dhess]

I have a few more suggestions related to git. Depending on how comfortable you are with git rebasing, maybe we should save these for when we next meet in person. I'm also happy not to actually fix any of these, and just bear these points in mind as guidance for the future, as long as @dhess feels the same way.

• We try to follow the Conventional Commits spec (but we aren't particularly strict about it). The commit 10b6a70 is missing a fix:/feat:/refactor: prefix.
• It's not super important, but commit messages are usually written in the imperative present tense. So, for example, change "Implemented" to "Implement".
• The commit history is slightly messy in places. For example, some of the changes in 10b6a70 seem like they'd belong better in 56183b2.

Rebasing is a pain and tricky even for experienced developers, and probably not a great use of either your or @patrickaldis's time, so I think we can make an exception in this instance.

[dhess]

One last thing — don't forget to update the first post to reflect what you want the merge commit to say, as that's what'll appear in the git logs.

[georgefst]

One last thing — don't forget to update the first post to reflect what you want the merge commit to say, as that's what'll appear in the git logs.

That was true with Bors, but not with GitHub's merge queues. See e.g. 5d2cd96.

[georgefst]

Was there any attempt here to align the prettyprinter colors with our frontend tree color scheme?

Yes.

[georgefst]

It's only Weeder failing now, so once the new tests I suggested (testing not and prettyType) are added, we'll be good to merge!

[georgefst]

Actually, one final request: could we use different colours for global variables and constructors?

It looks like Blue is our only unused colour (aside from Black, but I'm wary of using that as it clashes with most terminal backgrounds). And with local vars using Cyan that works out nicely, as they should be similar (in fact, it wouldn't bother me if they used the exact same colour, but seeing as we have one spare, we may as well use it).

[georgefst]

Some minor style issues (the kind of thing HLint would pick up, if it were slightly cleverer) with this function:

• Any particular reason why it isn't written in curried form? i.e. Why isn't the type String -> Def -> PrettyTestHandler -> TestTree?
• We bind n, then assign it to dname below. Why not just prettyTest (dname ...?

[patrickaldis]

I think the n/dname issue was just because I was writing all the variables in the definition as single letters, and then later changed it when using it in the the where block. I'll change this

I thought in the type signature

(String, Def) -> PrettyTestHandler -> TestTree

The expression name and definition came as a pair, whereas the handler seemed separate. I can make this curried though if this is bad practice.

[georgefst]

The expression name and definition came as a pair, whereas the handler seemed separate.

Fair enough! Sometimes that distinction does make sense.

[georgefst]

Rebased due to conflicts with #563.