prettyprinter icon indicating copy to clipboard operation
prettyprinter copied to clipboard

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

Open newhoggy opened this issue 3 years ago • 7 comments

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 avatar Sep 25 '21 06:09 newhoggy

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

newhoggy avatar Sep 25 '21 07:09 newhoggy

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.

sjakobi avatar Sep 27 '21 14:09 sjakobi

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.

Bodigrim avatar Sep 27 '21 22:09 Bodigrim

@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 avatar Sep 27 '21 23:09 newhoggy

@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.

newhoggy avatar Sep 27 '21 23:09 newhoggy

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.

Bodigrim avatar Sep 28 '21 05:09 Bodigrim

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.

newhoggy avatar Sep 28 '21 08:09 newhoggy