-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
python 3 compatibility #279
Conversation
Does this overlap at all with #43? |
I was not aware of the other pull request. The other one seems to contain more things (some of which I am not sure why they are necessaru) but outdated. This pull request contains the changes I had to make to get the rosdep command init and update working with Python 3.3 on Saucy. |
from urllib2 import urlopen | ||
from urllib2 import URLError | ||
try: | ||
import pickle as pickle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be cPickle. It provided a noteable speedup. I'm not sure if that still holds for python3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the pull request to try import cPickle as pickle
which was the initial idea of it.
One comment otherwise +1 |
In Python3 |
Python 3 unit tests failing. Looks like rosdistro needs to be ported first.
|
It's finally green. Please (re)review. |
+1 |
@@ -62,7 +62,7 @@ class RosdepInternalError(Exception): | |||
def __init__(self, e, message=None): | |||
self.error = e | |||
if message is None: | |||
self.message = traceback.format_exc(e) | |||
self.message = traceback.format_exc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work the same on Py26?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should work. The optional argument should be the limit
- never the exception itself. I guess 2.x was just nicer in handling this wrong argument.
+1 |
@tfoote @wjwwood Please review.