On Wed, Sep 18, 2013 at 10:09 AM, Simon Hengel <sol@typeful.net> wrote:

On Wed, Sep 18, 2013 at 09:02:16AM +0200, Bas van Dijk wrote:
> On 18 September 2013 06:30, Niklas Hambüchen <mail@nh2.me> wrote:
> > On 18/09/13 04:02, Dan Burton wrote:
> >> Everybody already uses, trusts, and loves aeson.
> >
> > There's only one thing about it that I don't trust:
> >
> > "Partial instances" that will call error in pure code and crash my program.
> >
> > ByteString has such an instance that simply assumes that ByteString is
> > the same as Text, so it calls Data.Text.Encoding.decodeUtf8 on it.
> >
> > https://github.com/bos/aeson/issues/126
>
> What about settling that issue by adding documentation to the ToJSON
> ByteString instance and the 'encode' function warning users about
> this?

That looks scary. I'd either make it total or remove the instance all
together.

I think by now almost everybody in the Haskell community agrees that we
do not want pure partial functions.

I don't want this issue to block inclusion of aeson. And IMO, the HP review process shouldn't be a time to try and twist a maintainer's arm into making a change he/she isn't interested in. That happened quite a bit on the text package, and I don't want to see it happen on aeson as well (especially given that Bryan would be the victim in both cases).

That said, I personally would prefer removing the ToJSON/FromJSON instances for ByteString. We've been bitten by this in our codebases, and have resorted to using a ByteString64 newtype wrapper, which uses base64 encoding to ensure safe roundtripping.

So: +1 from me on including aeson as-is, and if Bryan's interested in making this change to aeson, I'd consider it a perk.

Michael