search

appliedsec / rpctools

Project for providing JSON-RPC (and eventually XML-RPC) libraries with enhanced SSL support.
2 stars 4 forks source link

Preliminary Python 3 support #4

Closed ftobia closed 10 years ago

ftobia commented 10 years ago

I've got the (very basic) unit tests passing under Python 2.7 and Python 3.4. (As an aside, they should work on 3.3 too, but I need to reinstall it on my mac so I'm not trying it out just yet.)

I included the six.py file within the code base, since there weren't any dependencies and I figured there's no reason to add one now.

The only non-import non-syntax change had to do with URI parsing in the constructor of ServerProxy. The functions being used were internal to urllib, undocumented, and not in Python 3. I used urlparse and basically guessed what everything was. I would not trust any of that code until I write a bunch of tests for it.

Whoops, I forgot to make another branch for these changes. Also please forgive the copious whitespace trimming.

ftobia commented 10 years ago

I decided to merge in a Travis file and some other minor changes. I didn't think you guys would mind. Not sure if it works yet -- fingers crossed.

ftobia commented 10 years ago

https://travis-ci.org/ftobia/rpctools/builds/24390847 Yup looks good.

ftobia commented 10 years ago

It strikes me, as I re-read this commit https://github.com/ftobia/rpctools/commit/6b6c0f2e63bd4c8feee788d7cabbca5babd51d62 that I'm almost certainly wrong about what "auth" is supposed to be. Just a heads up, in case you know exactly what that code should look like.

EliAndrewC commented 10 years ago

Merging in travis was a good idea IMO.

EliAndrewC commented 10 years ago

Ok, at this point I've reviewed all of the commits, and everything looks good except for the auth part, which I commented on inline in the commit itself.

I'd feel better about merging this if we had just a few more unit tests. Just some basic tests of ServerProxy would probably be enough. From glancing through the source code, just instantiating and manually calling _request() would be good enough, which would only require a little bit of mocking; mostly you'd just need to mock out self.transport.request().

The auth stuff wouldn't even necessarily need a unit test, although I'd feed better if it did have some, since literally all that does is set the Authorization header: http://en.wikipedia.org/wiki/Basic_access_authentication#Client_side

ftobia commented 10 years ago

Working on this now. Hopefully I'll have an update tonight.

ftobia commented 10 years ago

This should do it. The behavior is slightly different: the Authorization header is now a bytes instead of a string, and the handler doesn't include the querystring anymore. I think those are okay changes but let me know what you think.

EliAndrewC commented 10 years ago

Looks great, I'm going to go ahead and merge this since the unit tests are all passing on @ftobia's travis (https://travis-ci.org/ftobia/rpctools) and the code looks good. If memory serves, only @hozn has the keys to PyPI and thus we'll need to coordinate with him to upload a new 0.2 version there, so I'll talk to him at work on Tuesday about that.

EliAndrewC commented 10 years ago

I take it back - Rob added me as an owner on rpctools in PyPI so I can tag and update that anytime without needing to coordinate with @hozn.