Python3 compat #26
Python3 compat #26
Conversation
|
This is great! Really happy to see someone working on Python 3 support. A couple of thoughts: Maybe it's worth borrowing from Request's and maintaining a compat module for Python 3 compatibility purposes? I'm a little concerned about some of the encoding changes, specifically where ASCII is explicitly called. Probably not a big deal but again, maybe this is best managed by breaking out Python 3 idioms into their own module where it makes sense? Anyway, again, very much appreciate the effort here! |
the request compat module looks nice, should be possible to reduce the imports to a single from request.compat import line using that. Not sure how public/stable that api is, but maybe rauth will follow request close enough so it wouldnt be a problem.
I, maybe falsely, assumed the tokens/keys would all be ascii. utf-8 is another option.
In my experience having common code for both is to prefer when possible. But encodings tend to be a bit tricky so maybe its better to leave them out in python2 where they are not needed.
Thanks and np |
Absolutely. Perhaps it's best to just add a new module to rauth that essentially replicates the functionality?
This sounds good to me. |
|
This looks awesome! Thanks for updating the pull request. Are tests passing for Python 3 as well? |
|
I think this is getting close: There's a syntax error on line 150 of oauth.py. Also I think it's worth cross checking everything by porting the example applications and making sure they work on both 2.x and 3. I tried with the Twitter example but there are a few outstanding changes that break the rauth API and probably need to be patched up before we can merge this. Otherwise this is looking really good. Thanks again! |
|
oops, messed up that last commit. there's a few tests not passing yet, and I'm trying to get a working py3.3 install where unicode literals are reintroduced to run the unicode tests. it's bound to be a few other issues but I hope to find those when I get a around to dog food the code. |
|
Sounds good. Let me know when you think it's ready to be merged. |
|
|
||
| :param s: object to be converted | ||
| ''' | ||
| return bytes(str(s), 'ascii') |
There was a problem hiding this comment.
Note that this doesn't quite do what you'd expect if s is already bytes; I don't know if that's a problem:
In [2]: s
Out[2]: b'abc'
In [3]: bytes(str(s), 'ascii')
Out[3]: b"b'abc'"
Something like this would handle that case:
if isinstance(s, str):
return s.encode('ascii')
return s
There was a problem hiding this comment.
And I've just seen that it's used on a float below. If you need to handle bytes as well, I guess this would work:
if isinstance(s, bytes):
return s
return bytes(str(s), 'ascii')
There was a problem hiding this comment.
Thanks for pointing this out. Certainly something that should be taken into consideration.
There was a problem hiding this comment.
good catch. I think I rather dumb it down a bit to only accept str and let the py2 version be a lambda x: x
to much magic will only hurt
There was a problem hiding this comment.
I agree about magic. :)
|
Also wanted to add that I'll incorporate Python 3 as a testing environment for our Travis configuration once this is finalized. |
| @@ -78,17 +77,20 @@ def __init__(self, response): | |||
|
|
|||
| @property | |||
| def content(self): | |||
| content = self.response.content | |||
| if not isinstance(content, basestring) and hasattr(content, 'decode'): | |||
| content = content.decode(self.response.encoding) | |||
There was a problem hiding this comment.
I think requests has a response.text attribute that does the same as this. I don't know what version of requests we require, though - it was added some time in the last few months.
There was a problem hiding this comment.
it's seems to have been introduced here, in january
kennethreitz/requests@49a38ac
Is there any reason to not use this for python2?
There was a problem hiding this comment.
We should be using response.text for Python 2. This was an oversight of my part.
There was a problem hiding this comment.
Also there's nothing stopping us from increasing the required version of Requests. So if there are other features that should be taken advantage of that's an option too.
|
Just a note that this will have to be rebased before it can be merged. This could happen after the implementation details are finalized or before. |
|
Hey great, looks like this is coming along nicely! Seems the Twitter example is now functional? |
|
Yup, it's working. finally got some time to look into this again. the unittests should work now with python3 after running 2to3, except for test_parse_utf8_qsl_non_unicode which becomes a bit weird when using response.text |
|
So is this essentially fully-functional at this point? Is it possible to fix that remaining unit test? |
|
Oh by the way, if you rebase off of master so that this can be merged, consider updating the .travis.yml config to include Python 3.x. This way Travis will test the unit test suite against Python 3. (I'd also like to ensure that Travis agrees this is good to pull.) |
|
How are things going with this? Do you think it's about ready to merge into master? |
|
Yeah, it all seems to work now. Not really sure how to get it to run on travis, it needs a 2to3 on the tests before running. |
|
Hm okay. Do the tests pass for 2.x after 2to3 or is it a mutually exclusive conversion? |
|
Also as a side note, it looks like Travis is not failing on coverage like it should be. So I'll need to update that. |
|
it's to bad, but after 2to3 the tests no longer work in py2. The change is in the unicode literals. |
|
What Python versions do you need to support? From Python 2.6, you can use Alternatively, there's the approach six offers, where you define a |
|
2.6 is the minimum version we need to support. |
|
@takluyver That worked nicely tests are now passing in both 2.7 and 3.2 without any modification |
|
How about 2.6? |
|
I don't have a 2.6 on this laptop, but the travis build was green. |
|
Unfortunately Travis is not failing (as it should be) for coverage. If you run the tests locally you'll see there's an issue with PEP8. Either we should ignore the compat module or fix those in some way. |
|
Here's the log for the curious (and those without 2.6): http://travis-ci.org/#!/litl/rauth/jobs/2573204 |
|
Any updates here? |
|
Checking in, wondering where things sit with this commit set? |
|
Sorry I didn't get back to you sooner, things been hectic. seems to be pyflakes (and not pep8) complaining about the unused imports, it's a quite easy workaround to add dummy statements using the names. complete coverage of the compat module I don't think we can get unless the results of a 2.x and 3.x run is combined |
Totally understand: can we just explicitly exclude it from coverage in the tests in that case? |
|
seems like it's crashing from a broken requests build. |
|
I see that none of the tests are running properly on Travis. Maybe we need to require a specific version of Requests if that's what you think seems to be causing the issue? I also see that on the 3.2 build pyflakes is not invoked properly... Coverage is missing on 2.7 and 2.6. |
|
Is there anything I can do to help out with this? Would love to get 3.x support in before the end of the year. Also working on doing a new release that will incorporate some breaking API changes, might be a good milestone to include this set of changes as well. |
* only encode hash input on python3 * decode content before trying to parse json
true division is the default in python3
ran 2to3 on the test files and then added a future import for the benefit of python2.
|
If you have any idea on how to fix the travis issues that would be great. And the pyflakes thing. |
|
Sure I'll see what I can do. |
|
Just a heads up: due to the API changes in requests v1.x, I've had to completely rework the OAuth1 architecture. Hopefully this won't make Python 3 support much more difficult than it already is but it probably does mean this patch has to be rewritten at least in part. |
|
Hi again, I'm going to close this pull as it won't be compatible with 0.5.0. However if you have time to do a rewrite against the new release of rauth, please feel free to reopen this or make another pull. |
|
I think there's some renewed interest in porting rauth to Python 3. Is anyone who had worked on this willing to pick things back up and try to see how well they mesh with the changes in rauth 0.5.0? |
These patches should make it possible to use rauth with python3 without breaking python2 support.
Not all unittests are working yet for python3. Most importantly not the unicode ones as I have to comment these out as the u'' stuff is a syntax error in 3.