Skip to content

Conversation

zachlowry
Copy link
Contributor

rather than ASCII strings.

@zachlowry
Copy link
Contributor Author

Can you review @brianjmiller? Thanks!

@brianjmiller
Copy link
Member

This seems to represent a pretty big change to the API, is this to handle binary data? It seems like it'd be better to determine the encoding based on the request headers and do it that way. I was somewhat confused on .decode and whether it is returning a byte stream or a string, if it is maintaining a byte stream then minimally we need to change the documentation to show whether to expect a 'unicode' string or a byte stream (or always a byte stream).

It'd be really nice to have a test exercising this bit of code to make sure it doesn't get switched back later.

@jakednoble do you have thoughts?

@zachlowry
Copy link
Contributor Author

This is to handle standard UTF-8 text in a statement. There's a couple of things here:
1: calling unicode(value) is a no-op. at the very minimum, to convert a string to a unicode, you must grab the return value of unicode().
2: the unicode() constructor expects an ASCII bytestream (also known as the 'str' type) and converts it as such. Therefore, if you have a UTF-8 character occur in this bytestream, the call throws an exception. .decode fixes this by forcing it to use the UTF-8 parser rather than the standard ASCII one. unicode(value) is essentially equivalent to value.decode("ascii").

It appears that the input to this function comes from the HTTPResponse object in httplib, which is populated with a byte stream.

@zachlowry
Copy link
Contributor Author

Test added! Back you you @brianjmiller.

@zachlowry
Copy link
Contributor Author

Sadly I can't assign you, @brianjmiller.

@brianjmiller brianjmiller self-assigned this Oct 28, 2014
brianjmiller added a commit that referenced this pull request Oct 28, 2014
Switch handling of non-Unicode responses to treat them as byte streams
@brianjmiller brianjmiller merged commit 8590c03 into RusticiSoftware:master Oct 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants