Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion instances for text Builder type #398

Open
chris-martin opened this issue Feb 1, 2022 · 5 comments
Open

Conversion instances for text Builder type #398

chris-martin opened this issue Feb 1, 2022 · 5 comments
Labels
question Further information is requested

Comments

@chris-martin
Copy link
Contributor

Is there a reason that the Data.Text.Lazy.Builder type does not make any appearance in Relude.String.Conversion? I think it would be useful to have ToText, ToLText, and ToString instances for it.

@chshersh chshersh added the question Further information is requested label Feb 1, 2022
@chshersh
Copy link
Contributor

chshersh commented Feb 1, 2022

@chris-martin No particular reason. I didn't need to do this often enough to have a need for introducing this to relude.

I imagine working with builders as constructing a builder from different parts and converting the final single value of type Builder to the desired representation once in the end for the output (e.g. printing in terminal or getting rendered HTML). In my experience, I almost never had a situation where I had different string types including Builder and I wanted to convert from Builder to one of these types. Usually, you convert types to Builder for more efficient string construction.

So, actually, the most benefit would be in having a typeclass like ToBuilder. But here we're entering the territory of many already created libraries in the space, and in relude we don't want to give preferences to one library over others:

Also, there're two builders: Text and ByteString with no particular winner of one over another. So we would have a typeclass like ToTextBuilder and ToByteStringBuilder but I feel like they'll quickly become "not enough" once you want to throw a few numbers in the final builder.

@chris-martin
Copy link
Contributor Author

Typically what I have is something like this:

  • Module A - Defines a whole bunch of functions that return Builders for different parts of a larger value
  • Module B - Imports one of the Builder functions from module A and immediately converts its result to text or bytestring.

Module A is in the situation you describe, where really it needs to import directly from text or bytestring because it needs all the particular facilities of that library to build numbers, etc., and so it does not particularly benefit from relude attempting to abstract over the underlying package because I already need a qualified import of Data.Text.Lazy.Builder for myriad reasons.

Module B, however, is the one that benefits from having the ToLText instance in relude - it would remove the need for this module to import the ....Builder module at all.

@chshersh
Copy link
Contributor

@chris-martin I understand your use-case as the following (so, please, correct me if I'm wrong):

  1. Module B imports one of Builder functions. Probably the "main" one or the "root" one for constructing the bigger value.
  2. Having ToText instances for Builder would allow you to write the following:
myText :: Text
-myText = toText $ Data.Text.Lazy.Builder.toLazyText myBuilder
+myText = toText myBuilder

I can see this as a UX improvement. But rather small one if you only have this toLazyText call once per module.

And I wonder if this change is justified. My current worry in adding instances of ToText / ToLText/etc. for different Builder types is accidental usage of them inside some big code block about building chunks of text and accidentally introduce performance regressions. For example, you created a pretty-printed version of some AST by adding Text values with <> and you probably used lots of toText functions. But now you want to switch to Builder and if you just change types and rely on GHC ability to catch errors and guide your TDD, you might be left with lots of toText calls that you need to cleanup manually.

Which is now less convenient and is bad UX.

So my current decision is to not introduce such instances and keep things as it is. But I'm not an active user of Builder types and I'd like to ask @Bodigrim and @Kleidukos for feedback (feel free both to ignore the request if you don't have the capacity for this).

@Kleidukos
Copy link

Kleidukos commented Apr 14, 2022

A little background on my case:

With feedback from the community of text-display users, I made the choice to have the methods of the Display typeclass return a Builder, whilst the display function proceeds to get the final Builder and then converts it to a Text:

display :: Display a => a -> Text
display a = TL.toStrict $ TB.toLazyText $ displayBuilder a

This has the advantage that the consuming site gets a Text (HTML templating, for example), while the accumulation of the Builders is done without converting back-and-forth until the consumer has the final Text.


Regarding the multiplicity of the Builder types, I also had to make an arbitrary choice between Bytestring and Text's builders; I think I'll adopt Bodigrim's builder when my support window allows it.


Should there be a support for Builder in the typeclasses of our beloved relude? For me the situation is very muddy and I'd rather have some experience report to look at so that I could provide more insight. :)

@Bodigrim
Copy link
Collaborator

I think it's a reasonable request, but take it with a grain of salt: I'm a fan of IsString / IsList and instance Foldable (a,) ;)

@chshersh chshersh pinned this issue Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants