Some comments

[minad]

Hi David,

I just saw the release of your package on hackage. Looks pretty nice! I did something similar a short while ago based on ekmetts wl-pprint-extras since I needed something working with annotations: https://github.com/minad/wl-pprint-annotated and https://github.com/minad/wl-pprint-console. However your package looks better and more ambitious, so I might switch over :)

Some comments:

• There is no Functor instance, but instead unAnnotate/reAnnotate. I find it much nicer to have a Functor instance since this is what I expect. What about adding one?
• You use Lists instead of Foldable everywhere probably to support type inference? I have usually a slight preference to the more general.
• I looked only shortly into your different rendering mechanisms, but displayDecorated and displayDecoratedA functions are only provided by the compat . These are the functions I am using to render the SimpleDoc in https://github.com/minad/wl-pprint-console/blob/master/src/Text/PrettyPrint/Console/WL.hs. However for ANSI coloring I am using some intermediate representation of another library which tries to generate only minimal escape sequences.
• There are some empty packages which essentially only reexport very few definitions. I think these confuse more than help when clicking through hackage or the source, like Internal.Type, ShowS etc.

Daniel

[quchen]

Hi Daniel,

thanks for the review! Let’s go through the list,

No Functor

I thought about this one, but decided it’s not a very obvious Functor instance so I left it away intentionally; I mention this in the documentation of reAnnotate. If you don’t agree and you were surprised by the lack of a Functor instance, I might consider adding it. As for unAnnotate, that’s not compatible with Functor, since we cannot construct a Functor f => f a -> f b value. Speaking of unAnnotate: I also found it unnatural to have »unAnnotate« be removing annotations, and the name-completely-unrelated »fmap« doing reannotation.

Lists instead of Foldable

Two reasons for this: type inference, and documentation readability. You can easily define your own concatenation functions with concatWith though, which was left Foldable-polymorphic.

Missing displayDecorated(A)

These cater to fairly special use cases and have an awkward API, so I replaced them with the generic stack-based/tree-based renderers. I don’t understand your use case quite enough to help you on this one, but it sounds like a good idea to reduce ANSI sequences – could you explain your workflow a bit more, maybe we can figure something out?

Empty packages

I think they do make sense, each for different reasons:

• Internal.Type exports Doc for adaptor purposes. It’s marked internal, but the docs provide a stability guarantee. I made this to not seal myself off from hacks and compatibility shims that people might find useful.
• ShowS is a rendering backend, namely Doc to ShowS, and as such it should be a Rendering module. Unfortunately, the Show instance of Doc is defined in terms of it, and renderShowS uses Doc, so the two are mutually dependent; this is resolved by putting the actual definition into the internal module, but providing public access only from another public module. I’m not sure how to improve this without losing the benefits :-/

Do you think adding a short description of each module to the package overview/readme might help? Along the line of a sketch like below, except that I should take more than a minute to create it:

.
└── Data
    └── Text
        └── Prettyprint
            ├── Doc
            │   ├── Internal # Only use it for hacking etc
            │   │   └── Type.hs
            │   ├── Internal.hs
            │   ├── Render # Convert SimpleDoc to rendered output
            │   │   ├── ShowS.hs
            │   │   ├── Text.hs
            │   │   ├── Tutorials # Obvious
            │   │   │   ├── StackMachineTutorial.hs
            │   │   │   └── TreeRenderingTutorial.hs
            │   │   └── Util
            │   │       ├── Panic.hs
            │   │       ├── SimpleDocTree.hs
            │   │       └── StackMachine.hs
            │   ├── Unicode.hs
            │   └── Util.hs
            └── Doc.hs

[minad]

Thanks for your quick answer!

1 Functor

Yes I missed this one, since I am already using it. I think for Haskeller it is quite natural to see that something is a functor. I also have noAnnotate in my library which is not a functor function. I think thats exactly the point reAnnotate vs unAnnotate, they are simplify different as such deserve different names :)

If a library author provides typeclasses I also like to use them since they give me additional laws. While we are on it, you might also derive Generic and NFData. Are there other commonly used and useful typeclasses for this library.

2 Foldable

It is a matter of taste. Many people also don't like foldable if you consider the length discussions on reddit/cafe. Since my package uses foldable I have to try your package first to see what bites me more often - having foldable or having lists. But I am also using OverloadedLists and OverloadedStrings, so maybe I am not providing a good datapoint ;) concatWith looks good!

3 displayDecorated

I would really like to have this one. My argument above was meant in the way that they are not special at all. All my display functions basically rely on it. You can take a short look at
my display functions.

Concerning the coloring - my worflow is as follows: I use displayDecorated to map the SimpleDoc to a list [Colored a]. See the type here. This list is then interpreted by a state machine in my coloring library.
The coloring function doesn't generate redundant sequences by keeping track of the color state and emitting sequences only when something changes.

4 Nearly empty packages

I understand that some of the packages are internal. Therefore you need these reexport modules. However at first sight it looked a bit complex. Maybe it could be a bit simpler. As it is now it introduces a too fine hierarchy in my opinion, Public, Half internal (the Internal.Types), Internal.

Adding a short description to the module would certainly help. But what counts mostly is that the modules reflects the internal structure. For example your StackMachine is clearly separated from the rest, while the ShowS module doesn't reflect the structure. But in the end this is a matter of taste and of your personal cost function. For me empty modules have a high cost (too open the extra file etc) and full modules have a very high cost. You probably know gc.cpp ;)

[quchen]

Functor

I think for Haskeller it is quite natural to see that something is a functor

I agree that we like to see everything of kind * -> * as a »maybe it’s a Functor«, but I remember being stumped by this myself when I first realized I had a lawful instance here ;-)

Generic, NFData

Generic is a good suggestion (and Typeable!), NFData I don’t think is a good idea, since it adds an additional dependency (on deepseq) and runs into the »NFData (a -> b)« problem. seqDoc might be worth a thought.

displayDecorated

My intended workflow for this would be that you implement your own renderer using the utilities provided in the StackMachine/SimpleDocTree modules. Are the tutorials not covering your use case? I think you can just write a renderer from SimpleDoc(Tree) to [Color].

half internal

I agree it’s not pretty, but I want to make clear that using Doc’s constructors is not something users should just go ahead and play around with. Exposing it at all is for a very specific use case I can point people to should they ask; for all others, I think »internal, don’t use or rely on« is good advice.

[minad]

• NFData and -> is a problem, I missed that. However depending on deepseq in general is for free since it comes with ghc.
• Typeable is not necessary since it is always there, right? You might want to consider Data however. But I am seldom using Data and syb.
• displayDecorated: Sure my usecase can be done without it, it just needs more code and case over SimpleDoc which I would like to avoid. Did you look at my display functions? There is also a short function for html rendering.

[quchen]

Those definitions in your WL module do look really simple. I guess adding them to the StackMachine module won’t hurt, it will then contain the stack machine monad it contains now, and a couple of simpler API functions.

[minad]

cool thx!

[quchen]

Done. Can we close this, or is there something else left to discuss? (If so, maybe a separate ticket is in order, otherwise the thread becomes too long.)

[minad]

Ok, thx for implementing the decorated function! I open some other issues.