-
Notifications
You must be signed in to change notification settings - Fork 280
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
Modernize Python 2 code to get ready for Python 3 #928
Conversation
@@ -1,16 +1,17 @@ | |||
#!/usr/bin/env python | |||
|
|||
from __future__ import print_function |
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.
This is not necessary in this case (see http://wiki.ros.org/python_2_and_3_compatible_code#Print_is_now_a_function).
Same below for many other cases.
@@ -74,7 +75,7 @@ def test_noproject(self): | |||
|
|||
out = self.cmake(CMAKE_PREFIX_PATH=self.installdir, | |||
expect=fail) | |||
print("failed as expected, out=", out) | |||
print(("failed as expected, out=", out)) |
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.
The extra parenthesis don't seem to make any sense. Why are they being introduced?
96e5ebc
to
977604a
Compare
The import keeps Python 2 developers from breaking Python 3 code because when the import is used then print without the parens is a syntax error in both Python 2 and Python 3. The import also deals gracefully with commas inside a print statement thus the double parens for trying to preserve the output... $ python2
|
test/network_tests/test_basics.py
Outdated
@@ -1,11 +1,12 @@ | |||
#!/usr/bin/env python | |||
|
|||
from __future__ import absolute_import |
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.
Why is this needed here?
Same question below.
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 understand. The wiki page I referenced mentions that. I just don't think it should be used when not necessary. Wasn't there one occasion where print is called with multiple arguments? In that the future import would make sense. But I can't see it anywhere anymore since you force pushed. |
.travis.yml
Outdated
# stop the build if there are Python syntax errors or undefined names | ||
- flake8 . --count --select=E9,F821,F822,F823 --show-source --statistics | ||
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide | ||
- flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics |
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.
Please move the check into a unit test so that it is being run locally too.
|
For that line future import would make sense. I doubt the intention in Python 2 was to print parenthesis around the arguments. |
e41b14e
to
87616dc
Compare
87616dc
to
25492b9
Compare
I used str.format() instead of the print_function import to make the intention explicit. |
Looks good to me now. Thank you for creating the patch and iterating on it. |
Make the minimal, safe changes required to convert the repo's code to be syntax compatible with both Python 2 and Python 3. There may be other changes required to complete a port to Python 3 but this PR is a minimal, safe first step.
Run: futurize --stage1 -w **/*.py
See Stage 1: "safe" fixes http://python-future.org/automatic_conversion.html#stage-1-safe-fixes