Python3 support : another take #16
Conversation
|
Thanks for taking a go at this @jlaine -- would you mind adding a test case the covers |
|
Hi, adding a test sounds like an excellent idea, however it doesn't look as though this method - with or without a body is currently being tested at all, so I'm struggling a bit with what the expected input / output is. |
|
@DavidMuller I have added the requested unit tests, feel free to change them if you had envisioned them differently. I also altered the README.md to mention python 3 support. It would probably be worth enabling travis tests against python 3.5, but I'll leave that for another PR once this has landed. |
| @@ -95,7 +100,7 @@ def __call__(self, r): | |||
|
|
|||
| # Create payload hash (hash of the request body content). For GET | |||
| # requests, the payload is an empty string (''). | |||
| body = r.body if r.body else '' | |||
| body = r.body if r.body else bytes() | |||
| payload_hash = hashlib.sha256(body).hexdigest() | |||
There was a problem hiding this comment.
python2 vs pytnon3 string compability is still a bit of an unknown for me. Could you explain why we run .encode('utf-8') on hashlib.sha256(canonical_request.encode('utf-8')).hexdigest() but not on payload_hash = hashlib.sha256(body).hexdigest()?
(A separte python3 PR, #14, for example runs a .encode(utf-8') option in this general area)
There was a problem hiding this comment.
The key is that PreparedRequest.body is never a string, it is either None or binary data (bytes), encoding has already been applied by the time it becomes a request body. In PR #14 encoding is applied conditionally, but the only case it makes sense is when we replace None by an empty string. Instead of this hack we do the right thing and set body to an empty "bytes".
|
Thanks for pushing tests @jlaine -- have you had a chance to test these changes on a production system that uses python3? I ran some cursory tests on a python2 production system, and things seemed OK |
|
Yes, I am using this with python3 for elasticsearch. I had started by naively applying "encode" in the line you pointed out, which worked for GETs (no body, which got replaced by an empty string) but bombed for POSTs as the body is already bytes and hence does not have an "encode" method. |
|
By the way you can quite easily run some python3 tests locally by creating a py3 virtualenv (virtualenv -p /usr/bin/python3 myenv) and installing requests + elasticsearch |
|
Thanks for all your work here @jlaine -- I appreciate it Will issue a new pypi release in the next hour or so and post back here |
|
Version 0.3.0 is on pypi |
|
Awesome, thanks a lot for reacting so fast!
|
This approach does not require any additional dependencies.