search

mrkkrp / req

An HTTP client library
Other
337 stars 40 forks source link

add TextResponse, textResponse, fix grammar typos #29

Closed ocramz closed 6 years ago

mrkkrp commented 6 years ago

I have applied this as 3045fdcce364418251cb2d146636c369c57612c8. I added more changes, let me know what you think. I'll add a couple of new tests later.

mrkkrp commented 6 years ago

On a second thought I'm not sure these wrappers is a good idea. We probably should also add ReqBodyText and ReqBodyLText now for request bodies as well. Maybe it's best to leave encoding-decoding business to the end user? Although on the other hand these wrappers may be handy...

mrkkrp commented 6 years ago

I've been thinking about this and it looks like the existing set of response interpretations/request bodies is general and symmetric (i.e. byte strings are very general, JSON is a must have thing, etc.). Text can be easily obtained from byte string bodies, using any encoding with help of the various functions from Data.Text.Encoding. I think it's better to drop the text interpretations as they are a rather special case and only support one encoding---UTF-8. Looks like a lot of boilerplate to have only to support a not very popular functionality that can be achieved using existing byte string interpretations.

ocramz commented 6 years ago

On one hand it's true that this new functionality is just a special case of the ByteString interpretation, but I figured I'd add it to req since I found myself adding it in one of my projects the other day.

I hope you'll want to retain it for future versions.

mrkkrp commented 6 years ago

I thought about this for a couple of days, and decided to remove the interpretations for now. I checked what other similar libraries do and they seem to be OK without this feature. (I've already mentioned other arguments above.)

You can still define these interpretations in your project locally, so it shouldn't be a problem.