-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
2to3: drop fix_print and avoid double parens in fix_print_with_import
- Loading branch information
Showing
3 changed files
with
18 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,17 @@ | ||
from libfuturize.fixes.fix_print_with_import import FixPrintWithImport | ||
from libfuturize.fixes.fix_print_with_import import FixPrintWithImport as FixPrintWithImportOrig | ||
from lib2to3.fixer_util import Node, Leaf, syms | ||
|
||
|
||
class FixPrintWithImport(FixPrintWithImportOrig): | ||
|
||
def match(self, node): | ||
isPrint = super(FixPrintWithImport, self).match(node) | ||
if isPrint and | ||
len(node.children) == 2 and \ | ||
isinstance(node.children[0], Leaf) and \ | ||
isinstance(node.children[1], Node) and node.children[1].type == syms.atom and \ | ||
isinstance(node.children[1].children[0], Leaf) and node.children[1].children[0].value == '(' and \ | ||
isinstance(node.children[1].children[-1], Leaf) and node.children[1].children[-1].value == ')': | ||
return False | ||
|
||
return ok |
c4d9b0a
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 guess this won't fix things since the script doesn't know if
print(a, b)
should be treated in a python 2 or python 3 way, the call in itself is just ambiguous.There are IMHO two approaches:
from __future__ import print_function
-clutterc4d9b0a
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 not just remove the with e.g.:
c4d9b0a
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.
And how will we know which files have already been sanitized?
Talking about portable code, it looks like
'{} {}'.format(a, b)
is preferable: http://stackoverflow.com/a/12382738/2319028c4d9b0a
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 do we need that?
2to3
ofprint(a)
don't change anything...c4d9b0a
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.
What I meant to say is: we cannot look at it from a purely technical perspective, we also have to keep track of which files have been sanitized for usage with 2to3 or updated to compatible code for us as developers.
c4d9b0a
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 do we need that?
2to3
ofprint(a)
don't change anything...+1 to use format :-)
c4d9b0a
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.
Or from another perspective (not only print but also range, unicode, pyqt...), what is our plan of action? I'm a bit lost at the moment.
So far I have (mostly) modified code in C++ files, tests and console.
What would be important from my perspective:
@jef-n you have already put quite a lot of work into the fixers, what is your opinion?
c4d9b0a
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.
My approach would have been to adapt the code so that it keeps working in py2/pyqt4 without modification, and works in pyqt5 with running 2to3 on the build directory. That got quite far (ie. console, db_manager, GdalTools, processing seem to work that way). The next step should have been adapting 2to3 more so that the translated code works in Py2/PyQt4 and Py3/PyQt5. But I didn't really get to that yet...
When that's done, we can run 2to3 on the source instead of the build directory and have it working for both setups.
c4d9b0a
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.
Would have been or would be?
Is this approach already broken by recent changes?
c4d9b0a
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.
Well, I also didn't follow it strictly. pyqt and signals are applied - but those should work for both setups anyway - and a lot of changes where to complete to moving to new style signals - and didn't have the problem that we need to track if they are applied or not - applying them again, shouldn't change anything.
c4d9b0a
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.
Do you think it would be safe to run the pyqt and signal fixers on all python files?
We could introduce a new CMake option to run the script on the build directory as part of the build process. This way we could also run it on travis and make testing easier.
c4d9b0a
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.
That's already done:
IOW only the new composerpolygon/line tests weren't migrated yet.
c4d9b0a
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.
See also f413046
c4d9b0a
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.
Hmm... I don't find a 2to3 Makefile target here.
c4d9b0a
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.
sure - that's my toplevel makefile - but it outputs what is done.