search

quchen / prettyprinter

A modern, extensible and well-documented prettyprinter.
BSD 2-Clause "Simplified" License
293 stars 34 forks source link

Need prettyprinter exports to convert between `String` and `Text` #207

Open newhoggy opened 2 years ago

newhoggy commented 2 years ago

I'm trying to write a patch to optparse-applicative to use prettyprinter instead.

One of the requirements is to not create a dependency on the text package.

The SText constructor uses Text which forces me to use Data.Text.pack and Data.Text.unpack which forces me to add a dependency on text.

If prettyprinter exports pack, unpack and Text, I believe it would solve my problem because I will be able to use it to convert to String do my work there and convert back.

newhoggy commented 2 years ago

This is what I had in mind: https://github.com/quchen/prettyprinter/pull/208

sjakobi commented 2 years ago

If prettyprinter exports pack, unpack and Text, I believe it would solve my problem because I will be able to use it to convert to String do my work there and convert back.

I wonder whether adding these functions to, say, Prettyprinter.Util.TextCompat would be sufficient. If versions for lazy text etc are needed, they could be named stringToText, stringToLazyText etc.

Bodigrim commented 2 years ago

Let it be clear that while I indeed wish for o-a do not depend on text, I am in no position to impose my wish onto maintainers of o-a or prettyprinter or anyone else. My understanding is that o-a can migrate to prettyprinter more or less as is, and an extended API is required only to enable colouring of o-a output. With all due respect, I do not quite understand why @newhoggy pushes both changes in a same PR (https://github.com/pcapriotti/optparse-applicative/pull/428).

FWIW pack is just Data.String.fromString, and unpack is read . show, so this does not warrant any additional exports.

newhoggy commented 2 years ago

@Bodigrim fromString works well. read . show doesn't seem good because it seems to me to impose needless overhead of escaping and quotation only to strip it out again. It's still useful to export Text which may be either a type alias String or Text or else otherwise I cannot give explicit type signatures in my code.

newhoggy commented 2 years ago

@sjakobi I'd be happy to export the minimum types and functions on an as need basis in Prettyprinter.Util.TextCompat starting with just pack, unpack and Text and will push a PR shortly.

Bodigrim commented 2 years ago

read . show doesn't seem good because it seems to me to impose needless overhead of escaping and quotation only to strip it out again.

I'm not quite convinced that this is a blocker for o-a: its workload is mostly short ASCII texts.

newhoggy commented 2 years ago

Turns out the situation is better than I feared in that I don't need unpack which would have fallen back to read . show.

My motivation for this is much weaker now. Thanks to @Bodigrim for leading me the right way in this.

As for the PR exporting Text, pack and unpack.

I'll leave it to the maintainers to decide its fate. If it is merged I will convert optparse-applicative to use unpack, otherwise I will continue to use fromString.