Use 'prettyprinter' library #67

georgefst · 2020-06-09T22:06:52Z

This is a big change, and definitely isn't ready to go in as-is, but it's ready for feedback.

All tests should pass right now, but only because I changed some of the expected outputs...

This closes #25 as obsolete, and should make #43, #56 and #66 easier to implement. Indeed I'm going to go off and implement #66 on a separate branch now, seeing as that's what led me to do all this in the first place...

pretty-simple.cabal

src/Text/Pretty/Simple.hs

georgefst · 2020-06-09T22:14:18Z

src/Text/Pretty/Simple/Internal.hs

@@ -14,6 +14,4 @@ module Text.Pretty.Simple.Internal
 import Text.Pretty.Simple.Internal.Color as X
 import Text.Pretty.Simple.Internal.ExprParser as X
 import Text.Pretty.Simple.Internal.Expr as X
-import Text.Pretty.Simple.Internal.ExprToOutput as X
-import Text.Pretty.Simple.Internal.Output as X


Two modules have gone, as we now render straight from Expr.

The remaining OutputPrinter should perhaps be renamed ExprPrinter, since the old Output type that was being printed there no longer even exists.

georgefst · 2020-06-09T22:17:47Z

src/Text/Pretty/Simple/Internal/Color.hs

@@ -39,15 +37,15 @@ import System.Console.ANSI
 --
 -- If you don't want to use a color for one of the options, use 'colorNull'.
 data ColorOptions = ColorOptions
-  { colorQuote :: Builder
+  { colorQuote :: AnsiStyle


Colours are now expressed with AnsiStyle. This should actually be easier for users to work with, but is obviously a breaking change.

georgefst · 2020-06-09T22:20:56Z

src/Text/Pretty/Simple/Internal/OutputPrinter.hs

+-- suitable for passing to any /prettyprinter/ backend.
+-- Used by 'Simple.pString' etc.
+layoutString :: OutputOptions -> String -> SimpleDocStream AnsiStyle
+layoutString opts = exprsToDocStream opts . expressionParse


This is potentially very useful, and should perhaps actually be exported from a non-Internal module somewhere.

It makes things like coloured HTML output much easier.

It makes things like coloured HTML output much easier.

Well, it would, except that AnsiStyle is opaque, and so it wouldn't be possible to define a conversion from it.

Not sure how best to resolve this. Perhaps have our own abstract Colour type?

src/Text/Pretty/Simple/Internal/OutputPrinter.hs

cdepillabout · 2020-06-10T11:21:20Z

@sjakobi Would you want to review this as well?

georgefst · 2020-06-10T11:22:55Z

As it happens, I just mentioned it to him on Reddit about two minutes ago.

sjakobi

Sorry, no proper review yet. I've had a brief look at the code and found it quite dense. Not sure how to make it more digestible though, maybe add type annotations and haddocks?

Or are you looking only for feedback on something particular at this stage?

src/Text/Pretty/Simple.hs

georgefst · 2020-06-12T09:52:29Z

Thanks @sjakobi. I was mostly just looking for feedback on the bits I've commented on here for now. I appreciate the full code isn't too easy to follow.

I should probably separate more of exprsToDoc in to annotated and Haddock-ed top-level definitions.

And I'll try to match the old output fully, while keeping in mind how the rules on spacing could be made configurable.

georgefst · 2020-06-24T16:49:59Z

I've finally found the time to revisit this, but cabal test is failing with:

Running 1 test suites...
Test suite pretty-simple-doctest: RUNNING...
pretty-simple-doctest: /tmp/bios-wrapper9254-14: getPermissions:getFileStatus: does not exist (No such file or directory)
Test suite pretty-simple-doctest: FAIL

I don't think I've changed anything relevant since this worked two weeks ago (I haven't changed anything within pretty-simple at all)... But I'm not very familiar with Cabal-doctest so I don't know how to start debugging this.

@cdepillabout @sjakobi Have you ever seen this sort of failure?

sjakobi · 2020-06-24T21:34:41Z

@georgefst Sorry, no idea regarding that failure. I mostly use doctest with stack (and without Cabal-doctest).

georgefst · 2020-06-26T10:53:11Z

This should now be a lot simpler and more readable.

Changes:

3 of the 5 test cases that were previously changed have been reverted:
- We now have the old compact behaviour back with singleton lists and tuples (i.e. no unnecessary newlines).
- The printing of multiline strings is still slightly less compact. I'm sure I could find some way to return the old behaviour if anyone feels strongly about it, but it would complicate things a little for questionable gain.
I've ended up using ViewPatterns and RecordWildCards, but these could be avoided if required, especially the latter.
I dropped the Stream dependency, and implemented a Tape data structure from scratch (for storing the state when colouring brackets). There are libraries on Hackage that could provide this, but it seems that it's always tied up with a lot of other functionality, and brings in a lot of transitive dependencies.

87984b2 and related changes introduce some conflicts, but it looks fairly straightforward to solve.

Finally, the commit history is a little messy. I strongly recommend this be squashed.

cdepillabout · 2020-06-26T11:48:59Z

@georgefst Could you rebase this on the current master?

Also, simplify the implementation considerably

georgefst · 2020-06-26T12:48:33Z

Could you rebase this on the current master?

@cdepillabout

Done, but there's one test failure. It looks like pretty-simple was previously trimming trailing newlines from string literals, which this PR doesn't do.

Is there a particular reason for doing that, or shall I just change the expected test result?

cdepillabout · 2020-06-26T13:14:46Z

@georgefst I actually don't really remember, but let me take a look at it when I review this PR.

(Sorry, I've been quite busy these last few weeks. It might take me another week or two til I have enough time to look through this.)

georgefst · 2020-06-26T13:21:28Z

Sorry, I've been quite busy these last few weeks. It might take me another week or two til I have enough time to look through this.

No worries at all! The work I actually needed this for required a further change anyway, so I'm happily using my own fork for that (though I will open a further PR once this is merged).

cdepillabout · 2020-06-26T13:32:47Z

Oh, and don't worry too much about trying to support all sorts of older GHCs. I can always fix this up if I feel like it, but I don't want you to have to spend too much time on this.

It is probably unlikely anyone absolutely needs new versions of pretty-simple on like GHC-8.2, GHC-8.0, GHC-7.10, etc!

georgefst · 2020-06-27T08:46:33Z

don't worry too much about trying to support all sorts of older GHCs

Yeah, it's not something I'd want to sink much time into, but it was an easy enough fix in this case (once I'd remembered the syntax...).

Looks like all the failures now are from that one test.

cdepillabout · 2020-06-27T09:16:01Z

@georgefst Okay, sounds good. So I guess you're ready for me to review this and possibly merge it in?

georgefst · 2020-06-27T09:26:43Z

I haven't yet had a chance to look at the issue with the extra indentation at the start of muti-line strings and 'other's (corresponding to the two modified test outputs).

But apart from that, yes. It's possible it could still do with more comments, but there are now at least haddocks on everything top-level.

georgefst · 2020-06-27T09:28:56Z

there are now at least haddocks on everything top-level

Well now there are.

PS. is it possible to tell the CI to ignore that sort of change? Just seems like such a waste of energy

cdepillabout · 2020-06-27T11:06:11Z

Okay, sounds good. I'll try to get to a review of this in a week or two or so.

is it possible to tell the CI to ignore that sort of change?

I think if you have commit rights here on github, it should be possible to stop CI, but unfortunately I didn't see this fast enough to be able to get to it.

More compact output for multiline strings. Expected output changed to reflect that trailing newlines aren't stripped.

georgefst · 2020-06-30T12:38:58Z

To be clear, the problem here is that we actually need Terminal.renderLazy to be lazy.

Lazy in the annotations? But it's supposed to render the annotations! I don't understand how that's supposed to fit together.

It's a stated goal of pretty-simple to be able to print partial results. e.g. pPrint [1,2,3, undefined] should still show the first few entries before failing.

The renderLazy name just refers to the output type: It's the lazy Text variant. renderStrict outputs the strict Text type.

Oh, okay. Then what's the purpose of having that variant if you can't take advantage of the laziness? Even something like TL.take 5 . renderLazy . fix $ SChar 'a' is ⊥, when, with a truly lazy renderLazy, it's "aaaaa".

georgefst · 2020-06-30T12:43:06Z

Anyway, we can fix this here with an actually-lazy renderLazy. I'm just surprised it's not the upstream behaviour.

As it happens, it's virtually a one-liner with quchen/prettyprinter#164, so perhaps I should reopen that...

sjakobi · 2020-06-30T13:04:16Z

It's a stated goal of pretty-simple to be able to print partial results. e.g. pPrint [1,2,3, undefined] should still show the first few entries before failing.

Oh, interesting! I wasn't aware of that.

The renderLazy name just refers to the output type: It's the lazy Text variant. renderStrict outputs the strict Text type.

Oh, okay. Then what's the purpose of having that variant if you can't take advantage of the laziness? Even something like TL.take 5 . renderLazy . fix $ SChar 'a' is ⊥, when, with a truly lazy renderLazy, it's "aaaaa".

I think it's just an exposed implementation detail that can be used when you don't need a strict Text. We make a Text Builder first, which is then converted to lazy Text. Maybe that conversion is less lazy than it ought to be!?

If you think that this is something that should be addressed, please do open an issue on the prettyprinter issue tracker.

georgefst · 2020-06-30T23:03:39Z

src/Text/Pretty/Simple.hs

+
+-- | Straightforward modification of 'Data.Text.Prettyprint.Doc.Render.Terminal'
+-- , in order to make it actually lazy
+renderLazy :: SimpleDocStream AnsiStyle -> TL.Text


I'd ideally like to upstream this to prettyprinter, rather than keep it here...

See quchen/prettyprinter#176

cdepillabout · 2020-07-14T11:34:50Z

Hey, I haven't forgotten about this, I just haven't really been able to find time for it within the past week or so! Hopefully I'll get a chance to take a good look at this in the next two weeks.

georgefst · 2020-07-17T15:58:10Z

@cdepillabout No worries, it's probably worth waiting for quchen/prettyprinter#176 anyway.

I still need to fix up some minor things with that PR, and I'm also waiting to see if the prettyprinter author replies.

georgefst · 2020-07-31T13:38:22Z

it's probably worth waiting for quchen/prettyprinter#176

And that's up! Many thanks @sjakobi

Allows us to remove laziness workaround.

Future-proofing

georgefst · 2020-07-31T15:00:48Z

CI has failed on The program 'stack' is currently not installed - pretty sure that has nothing to do with me

cdepillabout · 2020-08-11T12:46:09Z

@georgefst Thanks for doing so much work on this! Sorry for taking so long to get to the review.

I think this is a big improvement for the output printing, and I agree that with the nice refactoring you have here, a bunch of other PRs will be much easier to implement.

Do you want me to release this on Hackage as pretty-simple-4.0.0.0? Or pretty-simple-3.4.0.0? This is the biggest change made to pretty-simple in a long time, so I'm leaning towards 4.0.0.0.

georgefst · 2020-08-11T16:39:44Z

Well it's a big change, but it's mostly an internal one.

Personally I'd wait until some of the features and fixes which can make use of this change are implemented, seeing as these will bring more breaking changes, albeit not big ones.

I intend to submit PRs for the ones I care about in the next few days.

georgefst commented Jun 9, 2020

View reviewed changes

pretty-simple.cabal Outdated Show resolved Hide resolved

georgefst commented Jun 9, 2020

View reviewed changes

sjakobi reviewed Jun 11, 2020

View reviewed changes

src/Text/Pretty/Simple.hs Outdated Show resolved Hide resolved

src/Text/Pretty/Simple.hs Outdated Show resolved Hide resolved

georgefst added 7 commits June 26, 2020 13:16

Use 'prettyprinter' library for printing

20d8a2d

Fix multi-line 'other's

4625f30

Simplify and document annotation processing

da4ff87

Remove unused TemplateHaskell pragma

4b981a5

More compactness - fixes 3 of the tests

48a2359

Also, simplify the implementation considerably

Simplify further

19b1c18

Clean up and document

07f82ca

Import Data.Monoid on older GHCs

11f99b2

Fix CPP syntax

3b44727

Haddock-ify a comment

3d2460e

Fix remaining tests

c1b084d

More compact output for multiline strings. Expected output changed to reflect that trailing newlines aren't stripped.

georgefst mentioned this pull request Jun 30, 2020

Terminal.renderLazy isn't lazy quchen/prettyprinter#175

Closed

Make 'pString' lazy again

95a86f6

georgefst commented Jun 30, 2020

View reviewed changes

[skip ci] Fix typo

c1cc680

georgefst added 5 commits July 31, 2020 14:55

Bump prettyprinter-ansi-terminal version

f5700ca

Allows us to remove laziness workaround.

Remove ansi-terminal dependency

0de7092

Use the new prettyprinter module names

846b488

Future-proofing

[skip ci] Alphabetise extensions

88f6a16

Add stack extra-deps

2cc2093

cdepillabout added 6 commits August 11, 2020 21:14

Add note about stack extra-deps.

21f9804

Update stack.yaml.lock.

56ad278

Add CI workflows.

5d78be9

Make sure to compile with the correct flag.

aea27b3

Update the build status shield on the README.

c91b35e

Remove .travis.yml.

9566022

cdepillabout approved these changes Aug 11, 2020

View reviewed changes

cdepillabout merged commit d775103 into cdepillabout:master Aug 11, 2020

georgefst mentioned this pull request Aug 13, 2020

Make output less vertical #43

Closed

This was referenced Aug 22, 2020

Too many spaces #75

Closed

OutputPrinter module name #76

Closed

georgefst mentioned this pull request Jun 14, 2022

Compact mode destroys laziness #113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use 'prettyprinter' library #67

Use 'prettyprinter' library #67

georgefst commented Jun 9, 2020 •

edited

Loading

georgefst Jun 9, 2020

georgefst Jun 9, 2020

georgefst Jun 9, 2020

georgefst Jun 26, 2020

cdepillabout commented Jun 10, 2020

georgefst commented Jun 10, 2020

sjakobi left a comment

georgefst commented Jun 12, 2020

georgefst commented Jun 24, 2020

sjakobi commented Jun 24, 2020

georgefst commented Jun 26, 2020

cdepillabout commented Jun 26, 2020

georgefst commented Jun 26, 2020

cdepillabout commented Jun 26, 2020

georgefst commented Jun 26, 2020

cdepillabout commented Jun 26, 2020

georgefst commented Jun 27, 2020 •

edited

Loading

cdepillabout commented Jun 27, 2020

georgefst commented Jun 27, 2020

georgefst commented Jun 27, 2020

cdepillabout commented Jun 27, 2020

georgefst commented Jun 30, 2020

georgefst commented Jun 30, 2020

sjakobi commented Jun 30, 2020

georgefst Jun 30, 2020

georgefst Jul 1, 2020

cdepillabout commented Jul 14, 2020

georgefst commented Jul 17, 2020 •

edited

Loading

georgefst commented Jul 31, 2020

georgefst commented Jul 31, 2020

cdepillabout commented Aug 11, 2020 •

edited

Loading

georgefst commented Aug 11, 2020

Use 'prettyprinter' library #67

Use 'prettyprinter' library #67

Conversation

georgefst commented Jun 9, 2020 • edited Loading

georgefst Jun 9, 2020

Choose a reason for hiding this comment

georgefst Jun 9, 2020

Choose a reason for hiding this comment

georgefst Jun 9, 2020

Choose a reason for hiding this comment

georgefst Jun 26, 2020

Choose a reason for hiding this comment

cdepillabout commented Jun 10, 2020

georgefst commented Jun 10, 2020

sjakobi left a comment

Choose a reason for hiding this comment

georgefst commented Jun 12, 2020

georgefst commented Jun 24, 2020

sjakobi commented Jun 24, 2020

georgefst commented Jun 26, 2020

cdepillabout commented Jun 26, 2020

georgefst commented Jun 26, 2020

cdepillabout commented Jun 26, 2020

georgefst commented Jun 26, 2020

cdepillabout commented Jun 26, 2020

georgefst commented Jun 27, 2020 • edited Loading

cdepillabout commented Jun 27, 2020

georgefst commented Jun 27, 2020

georgefst commented Jun 27, 2020

cdepillabout commented Jun 27, 2020

georgefst commented Jun 30, 2020

georgefst commented Jun 30, 2020

sjakobi commented Jun 30, 2020

georgefst Jun 30, 2020

Choose a reason for hiding this comment

georgefst Jul 1, 2020

Choose a reason for hiding this comment

cdepillabout commented Jul 14, 2020

georgefst commented Jul 17, 2020 • edited Loading

georgefst commented Jul 31, 2020

georgefst commented Jul 31, 2020

cdepillabout commented Aug 11, 2020 • edited Loading

georgefst commented Aug 11, 2020

georgefst commented Jun 9, 2020 •

edited

Loading

georgefst commented Jun 27, 2020 •

edited

Loading

georgefst commented Jul 17, 2020 •

edited

Loading

cdepillabout commented Aug 11, 2020 •

edited

Loading