AJChapman / formatting

Format strings type-safely with combinators
BSD 3-Clause "New" or "Revised" License
164 stars 39 forks source link

Add a formatter to String #10

Closed nikita-volkov closed 9 years ago

nikita-volkov commented 9 years ago

I get your desire to force users not to use String, but then there are lots of APIs we all have to deal with, which still require String as an input. So providing a formatter to String seems a quite logical thing to expect from the API.

Here's a function I wrote in the module I use "formatting" in, but I really that this library is a much more proper place for it:

import qualified Data.Text.Lazy as TL
import qualified Data.Text.Lazy.Builder as TLB

formatToString :: Holey TLB.Builder String a -> a
formatToString =
  flip runHM $ TL.unpack . TLB.toLazyText

While we're at it, I also think that execution function titles like sformat are anything but descriptive. And since due to #9 an API-breaking change is coming, how about a change?

chrisdone commented 9 years ago

I get your desire to force users not to use String, but then there are lots of APIs we all have to deal with, which still require String as an input. So providing a formatter to String seems a quite logical thing to expect from the API.

I'm not strongly against adding a string formatter, I just haven't had a need to support it.

While we're at it, I also think that execution function titles like sformat are anything but descriptive. And since due to #9 an API-breaking change is coming, how about a change?

What would you suggest?

nikita-volkov commented 9 years ago

What would you suggest?

  • formatToString
  • formatToBuilder
  • formatToText
  • formatToLazyText
  • formatToByteString
  • ...

Concerning the print functions I actually have a feeling that they are redundant, but this could be due to me having no use for them in my case, so I'm not opinionated about that.

chrisdone commented 9 years ago

Those names are discouragingly long.

chrisdone commented 9 years ago

We could generalize this with a FromBuilder class, but that breaks type inference on other polymorphic code.

chrisdone commented 9 years ago

How about a compromise. See this new-names branch.

We add the FromBuilder class to retain the convenient format name, but now it works for builders or text or string or IO. But, when you're dealing with a polymorphic function (e.g. toHtml) then you can use formatT or whatever monomorphic type so that you don't have to add a type signature which I consider a real bummer.

nikita-volkov commented 9 years ago

I actually think that it's best not to use typeclasses solely for the reason of overloading. I think it is a common wisdom amongst the community. You've already given a practical reason why it is so.

Well, I don't know. I really don't think that the proposed names are that long. OTOH I myself prefer to write all APIs with the presumption that they will be imported qualified. If it was your case, all the names could be trimmed:

But then I don't want to sound like a wise guy. It's all up to you, of course. It's a very nice package either way.

chrisdone commented 9 years ago

I've just added your formatToString because no decision here will leave me feeling happy.