search

silkapp / rest

Packages for defining APIs, running them, generating client code and documentation.
http://silkapp.github.io/rest
390 stars 52 forks source link

Switch to ExceptT #104

Closed bergmark closed 9 years ago

bergmark commented 9 years ago

This fixes most of #78

I replaced all usages of ErrorT, and removed unneeded Error instances. Migration is easy for users so I think there's little benefit in keeping ErrorT anywhere.

I needed to add a Monoid instance for DataError. Any thoughts on what the definition should be?

I also could not upgrade rest-client (this does not prevent us from releasing this), it depends on https://github.com/ekmett/exceptions/pull/38 and possibly other libraries.

hesselink commented 9 years ago

I don't think we should add a Monoid DataError instance. Instead we should map Last (or First, if we decide that's better) over it at the use site to get the previous semantics.

bergmark commented 9 years ago

Ok I'm using Last instead, I think that's better than First, but didn't think about it that much.

This did cause an impossible case to turn up, we never throw Last Nothing. Is there anything better than error "impossible" we can do there?

hesselink commented 9 years ago

Hmm, annoying, that constraint should be Semigroup instead... I don't think it will ever turn up, so a call to error (with a slightly better message ;)) seems ok.

On the other hand, it seems memtpy is used for mzero/empty. Perhaps we should make sure that works, even though we don't use it currently, we might accidentally introduce it. We could add a constructor to DataError, or use something like ParseError "mzero" but that feels wrong to me.

bergmark commented 9 years ago

What do you mean by "Perhaps we should make sure that works"?

hesselink commented 9 years ago

Make sure that mzero works, i.e. isn't error. Just in case we use it in the future. OTOH, it used to be an infinite loop (since we had an empty implementation of Error, so it can't get much worse :))

bergmark commented 9 years ago

I'll merge this without a working mzero since I can't figure out a good way to do it. I'm not releasing this today so we can try to figure something out if you'd like.

822f581