Encoding updates #234
Encoding updates #234
Conversation
…est guess from mime type
| @headers = HTTP::Headers.coerce(headers || {}) | ||
| @encoding = encoding | ||
|
|
||
| if connection_or_body.is_a? HTTP::Connection |
There was a problem hiding this comment.
This didn't seem great, but I couldn't see a cleaner way to do it nicely.
There was a problem hiding this comment.
To the above, moving this to a hash means just setting it based on key rather than having to do content type detection.
|
This seems fine. But assumption 3 seems risky, but then again the entire HTTP spec is a mess. Someone else want to weigh in? |
|
Although HTTP spec is a mess, if it has declared defaults we should stick
|
|
I think http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7.1 is the relevant RFC, from what I can google I'm not sure anyone actually honors this though. I think most clients do some kind of character detection (which could also be added to http.rb, but I wanted to avoid it in this change if possible). |
|
Looking at Net::HTTP, it appears they just read directly to the string via |
|
Well assuming its binary seems like a good last resort, but I think we can do a bit better. At least step 1 and 2 above seem pretty solid to me. I could just remove 3 if that makes everyone more comfortable, or I'm happy to make it use ISO-8859-1 if thats preferred. |
|
Ok, I've removed 3 for now, I think its a good point that utf-8 things are probably more likely to declare their encoding anyway. |
| @headers = HTTP::Headers.coerce(opts.fetch(:headers, {})) | ||
| @encoding = opts.fetch(:encoding, nil) | ||
|
|
||
| if opts.include?(:connection) |
There was a problem hiding this comment.
I don't like this magic. Response should receive only :body. And this Body streamer should be left where it is now.
There was a problem hiding this comment.
I would have done that but to set the encoding in the body I need to have the response object. Do you have a better suggestion for how to organize that?
|
LGTM (with fixing response class) |
|
@Connorhd looks like this needs a rebase. Want to finish it up before we release 1.0? |
|
I'm working on this already :D |
|
In other words this will be in 1.0 for sure :D |
|
After thinking a bit more, all further cleanups can be made in future patch releases, so I'm gonna merge #273 (this rebased PR) as is. |
Should resolve #224
Adds an encoding option and sets the body encoding based on this order of priorities: