Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
2to3: drop fix_print and avoid double parens in fix_print_with_import
  • Loading branch information
jef-n committed Apr 1, 2016
1 parent b466c63 commit c4d9b0a
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 3 deletions.
2 changes: 1 addition & 1 deletion scripts/addfix.pl
Expand Up @@ -48,9 +48,9 @@
libfuturize.fixes.fix_metaclass
libfuturize.fixes.fix_next_call
libfuturize.fixes.fix_object
libfuturize.fixes.fix_print_with_import
libfuturize.fixes.fix_raise
libfuturize.fixes.fix_xrange_with_import
libpasteurize.fixes.fix_newstyle
/;

Expand Down
1 change: 0 additions & 1 deletion scripts/qgis_fixes/fix_print.py

This file was deleted.

18 changes: 17 additions & 1 deletion scripts/qgis_fixes/fix_print_with_import.py
@@ -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

15 comments on commit c4d9b0a

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 1, 2016

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:

  • Keep track of what we updated to python 3 (would be good also in order to not redo the work)
  • "Tag" files which have been updated with a from __future__ import print_function-clutter

@sbrunner
Copy link
Contributor

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.:

print("%s %s" % (a, b))

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 1, 2016

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/2319028

@sbrunner
Copy link
Contributor

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?

Why do we need that?

2to3 of print(a) don't change anything...

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 1, 2016

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.

@sbrunner
Copy link
Contributor

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?

Why do we need that?

2to3 of print(a) don't change anything...

+1 to use format :-)

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 1, 2016

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.

  • Leave the python code untouched and postpone running the fixers to "sometime in the future"
  • Leave the python code untouched and add some logic that runs the fixers as part of the installation step
  • For the above two: if we find code that needs manual intervention, how do we deal with it?
    • Fix it when we stumble upon
    • Fix it in a systematic way (module by module, tag which modules have been updated)
  • Apply random fixers on random files and hope that in the end we applied all fixers on all files and nowhere twice where it would hurt
  • Go module by module (or plugin by plugin), apply the fixers, test with python 2 and python 3 and tag it (in the metadata or in the python file) as upgraded and tested
  • Something different?

So far I have (mostly) modified code in C++ files, tests and console.

What would be important from my perspective:

  • Don't do the work twice
  • In terms of different people working on the same py files because they are unaware of what already happened
  • In terms of not upgrading once to "translatable" code and in 6 months to "py3" code
  • Develop a migration guide for the bazillions of plugins out there

@jef-n you have already put quite a lot of work into the fixers, what is your opinion?

@jef-n
Copy link
Member Author

@jef-n jef-n commented on c4d9b0a Apr 1, 2016

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.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 1, 2016

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?

@jef-n
Copy link
Member Author

@jef-n jef-n commented on c4d9b0a Apr 1, 2016

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.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 1, 2016

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.

@jef-n
Copy link
Member Author

@jef-n jef-n commented on c4d9b0a Apr 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's already done:

$ make 2to3
find . -name "*.py" \
    ! -path "./cmake/FindPyQt?.py" \
    ! -path "./python/utils.py" \
    ! -path "./python/PyQt/*" \
    ! -path "./debian/*" \
    ! -path "./python/plugins/processing/algs/qgis/scripts/*" \
    ! -path "./python/ext-libs/*" \
    -print | \
    xargs scripts/2to3 -j8 -w -f pyqt -f signals --no-diffs
root: Generating grammar tables from /usr/lib/python2.7/lib2to3/PatternGrammar.txt
root: Generating grammar tables from /usr/lib/python2.7/lib2to3/PatternGrammar.txt
RefactoringTool: Refactored ./tests/src/python/test_qgscomposerpolyline.py
RefactoringTool: Refactored ./tests/src/python/test_qgscomposerpolygon.py
RefactoringTool: No files need to be modified.
$ git diff
diff --git a/tests/src/python/test_qgscomposerpolygon.py b/tests/src/python/test_qgscomposerpolygon.py
index 926cb5f..1a070ea 100644
--- a/tests/src/python/test_qgscomposerpolygon.py
+++ b/tests/src/python/test_qgscomposerpolygon.py
@@ -14,9 +14,9 @@ __revision__ = '$Format:%H$'

 import qgis

-from PyQt4.QtGui import QColor
-from PyQt4.QtGui import QPolygonF
-from PyQt4.QtCore import QPointF
+from PyQt.QtGui import QColor
+from PyQt.QtGui import QPolygonF
+from PyQt.QtCore import QPointF

 from qgis.core import (QgsComposerPolygon,
                        QgsComposerItem,
diff --git a/tests/src/python/test_qgscomposerpolyline.py b/tests/src/python/test_qgscomposerpolyline.py
index 0239684..27ff284 100644
--- a/tests/src/python/test_qgscomposerpolyline.py
+++ b/tests/src/python/test_qgscomposerpolyline.py
@@ -14,9 +14,9 @@ __revision__ = '$Format:%H$'

 import qgis

-from PyQt4.QtGui import QColor
-from PyQt4.QtGui import QPolygonF
-from PyQt4.QtCore import QPointF
+from PyQt.QtGui import QColor
+from PyQt.QtGui import QPolygonF
+from PyQt.QtCore import QPointF

 from qgis.core import (QgsComposerPolyline,
                        QgsComposerItem,

IOW only the new composerpolygon/line tests weren't migrated yet.

@jef-n
Copy link
Member Author

@jef-n jef-n commented on c4d9b0a Apr 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also f413046

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on c4d9b0a Apr 4, 2016

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.

$ make 2to3
make: *** No rule to make target '2to3'.  Stop.

@jef-n
Copy link
Member Author

@jef-n jef-n commented on c4d9b0a Apr 4, 2016

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.

Please sign in to comment.