Python3 compatibility fixes by jdeschenes · Pull Request #284 · enthought/enable

jdeschenes · 2017-07-25T01:08:09Z

This is a fix to remove 2to3 when installing enable.

* print should now be functions instead of statements * raises should now respect the new syntax * "<>" operator was replaced by "!="

* Fixed tuple unpacking issues * replaced has_key with "in" syntax * exec is now a function instead of a statement

* Fixed __metaclass__ compatibility using six.add_metaclass

* replaced xrange by six.moves.range

* Fixed tuple unpacking issues * Fixed izip issues using six

* Replaced self.assert_ with self.assertTrue

* Fixed several non-trivial list vs iterables in kiva

* Fixed several non-trivial list vs iterables in enable

* Fixed reduce function using six * Fixed a few other issues not seen previously

* fixed unicode and unichr calls

* Fixed StringIO with six

* Fixed imports * Fixed some StringIO that were missed in the initial pass * Some string fixes

* Fixed issues with basestring * fixed issues with long

* Some fixes with import *

Solved some issues while running unit test.

# Conflicts: # enable/savage/compliance/comparator.py # enable/savage/compliance/viewer.py # enable/savage/svg/attributes.py # enable/savage/svg/css/colour.py # enable/savage/svg/document.py # enable/savage/svg/tests/test_document.py # enable/savage/trait_defs/ui/qt4/svg_editor.py # kiva/agg/src/graphics_context.i # kiva/fonttools/fontTools/misc/textTools.py # kiva/fonttools/fontTools/ttLib/__init__.py # kiva/fonttools/fontTools/ttLib/macUtils.py # kiva/fonttools/fontTools/ttLib/sfnt.py # kiva/fonttools/fontTools/ttLib/tables/DefaultTable.py # kiva/fonttools/fontTools/ttLib/tables/__init__.py # kiva/fonttools/fontTools/ttLib/tables/_n_a_m_e.py # kiva/fonttools/font_manager.py # kiva/fonttools/misc/textTools.py # kiva/pdfmetrics.py # kiva/ps.py # kiva/quartz/setup.py # kiva/svg.py # kiva/tests/test_macport.py # setup.py

codecov-io · 2017-07-25T01:17:09Z

Codecov Report

Merging #284 into master will increase coverage by 0.32%.
The diff coverage is 38.26%.

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
+ Coverage   33.26%   33.59%   +0.32%     
==========================================
  Files         206      206              
  Lines       18572    18278     -294     
  Branches     2522     2407     -115     
==========================================
- Hits         6178     6140      -38     
+ Misses      12001    11745     -256     
  Partials      393      393

Impacted Files	Coverage Δ
enable/tools/hover_tool.py	`0% <ø> (ø)`	⬆️
kiva/fonttools/afm.py	`45.11% <ø> (ø)`	⬆️
enable/wx/constants.py	`0% <ø> (ø)`	⬆️
enable/tools/api.py	`0% <ø> (ø)`	⬆️
enable/savage/trait_defs/ui/svg_button_editor.py	`0% <0%> (ø)`	⬆️
enable/scrollbar.py	`0% <0%> (ø)`	⬆️
enable/savage/compliance/comparator.py	`0% <0%> (ø)`	⬆️
enable/canvas.py	`27.36% <0%> (ø)`	⬆️
enable/vtk_backend/vtk_window.py	`0% <0%> (ø)`	⬆️
enable/tools/viewport_pan_tool.py	`0% <0%> (ø)`	⬆️
... and 76 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73829e9...812b243. Read the comment docs.

jdeschenes · 2017-07-25T01:20:57Z

The failure doesn't have anything to do with the actual PR. Should probably resolved separately.

jdeschenes · 2017-07-25T15:40:03Z

The travis-ci failure has been resolved in #285.

jwiggins · 2017-07-26T18:10:37Z

-    raise TraitError, ( object, name, 'a font descriptor string',
-                        repr( value ) )
+    raise TraitError(( object, name, 'a font descriptor string',
+                        repr( value ) ))


There's one set of parentheses too many here.

raise TraitError(object, name, 'a font descriptor string', repr( value ))

jdeschenes · 2017-07-26T19:43:40Z

I fixed the issue you found and another that I saw from scanning the code.

jwiggins

Did a quick pass through all the changes. Couple general comments:

If code is already using keys, values, or items, then I wouldn't worry about switching to an iter* function from six. Just wrap a list or dict around the places which don't expect an iterator and call it a day.
Try not to change the structure of the code. Our test coverage is not very good in this project and the minimal changes needed here are quite enough churn for a single pull request

jwiggins · 2017-07-26T18:25:17Z

    'long dash': array( [ 9.0, 5.0 ] )
 }
-__line_style_trait_map_keys = __line_style_trait_values.keys()
+__line_style_trait_map_keys = list(six.iterkeys(__line_style_trait_values))


Why not just list(__line_style_trait_values.keys())?

For efficiency

It runs once when the module is imported, so I wouldn't worry about it

jwiggins · 2017-07-26T18:25:59Z


 import math

+import six


unused import

jwiggins · 2017-07-26T18:31:16Z


        if event_type == 'character':
-            key = unicode(event.text())
+            key = six.u(event.text())


key = six.text_type(event.text())

jwiggins · 2017-07-26T18:31:38Z


 import warnings

+import six


unused import

jwiggins · 2017-07-26T18:34:41Z

    def __init__(self, parent, *args, **kwargs):
        wx.Frame.__init__(self, parent, *args, **kwargs)
-        self.pathOps = dict((k,v) for (k,v) in wx.GraphicsPath.__dict__.iteritems() if k.startswith("Add"))
+        self.pathOps = dict((k,v) for (k,v) in six.iteritems(wx.GraphicsPath.__dict__) if k.startswith("Add"))


Just use a regular dictionary comprehension:
self.pathOps = {k: v for k, v in six.iteritems(wx.GraphicsPath.__dict__) if k.startswith("Add")}

jwiggins · 2017-07-26T20:39:33Z

        Requires the Python Imaging Library (PIL).
        """
-        from kiva.compat import pilfromstring, piltostring
+        from kiva.compat import pilfromstring, piltostring, Image as PilImage


Why the Image as PilImage import?

It's used in the function but never imported in the module.(Line 249). I can move the import at the top of the module, but that could have more important impact.

Ugh, yep. I see the NameError now... Carry on.

jwiggins · 2017-07-26T20:41:00Z

-font_map = {'Arial': 'Helvetica',
-            }
-import _fontdata
-


Why did you remove this?

A few lines below, it is being imported. font_map is also defined a bit below.

That's just sloppy... Carry on.

jwiggins · 2017-07-26T20:42:14Z


    def create_graphics_context(self, width, height):
-        return GraphicsContext((width, height))
+        return GraphicsContext((width, height), pix_format="rgba32")


What's the reason for the new keyword arg?

jwiggins · 2017-07-26T20:42:47Z

-        raise TraitError, ( object, name, 'a font descriptor string',
-                            repr( value ) )
+        raise TraitError(( object, name, 'a font descriptor string',
+                            repr( value ) ))


Too many parentheses here

jwiggins · 2017-07-26T20:43:13Z

-          # with builtins) in the auto-generated SWIG files like
-          # kiva/agg/agg.py.
-          use_2to3_exclude_fixers=['lib2to3.fixes.fix_imports'],
+          use_2to3=False,


jwiggins · 2017-07-26T20:51:48Z

In general: Thanks!!! 💯 This is important work and we really appreciate you taking it on (here and in the other ETS libraries).

I'm a bit swamped with other things right now, so I haven't had a chance to do anything more than read through these changes. Hopefully someone else can take a look...

* Removed unused imports * Changed six.u to six.text_type * Replaced call with a dictionary comprehension in drawer.py * Fixed an an issue with a callable in svg_regex.py * Removed sm.zip in favour of the builtin function * Changed print("") to print() * Removed a pix_format keyword call. Still missing: * Corrections for itervalues, iterkeys and iteritems * Some clarifications for the comments that were written * `pdfmetrics.parseAFMFile`

jdeschenes

I have a few comments in the lines, I will need your input to move forward. As I said in the comments, I tried to keep the code structure to the minimum but there are few places where it simply isn't possible. I tried to fix a few bugs as I saw along the way.

As mentionned in my commit message,

pdfmetrics will require a bit of a refactor
I left out the iteritems, itervalues, iterkeys for later.
Some of the comments I left out there, might better belong in github issues? Let me know.

jdeschenes · 2017-07-27T00:29:15Z

            self._pass_event_to_vtk(vtk_obj, eventname)

    def _create_key_event(self, vtk_event, event_type):
+        # FIXME: THIS IS A BUG


The function just won't work. It is asking for two variables vtk_obj and eventname but they are never defined.

jdeschenes · 2017-07-27T00:30:08Z


    def _window_paint(self, event):
        "Do a GUI toolkit specific screen update"
+        # FIXME: THIS IS A BUG


A bit of the same... The function requires a variable union_bounds but it never defined.

jdeschenes · 2017-07-27T00:30:13Z

            key = KEY_MAP.get(self.control.key_sym, None)
            if key is None:
-                key = unicode(self.control.key_sym)
+                key = six.u(self.control.key_sym)


jdeschenes · 2017-07-27T00:30:14Z


        if event_type == 'character':
-            key = unicode(self.control.key_sym)
+            key = six.u(self.control.key_sym)


jdeschenes · 2017-07-27T00:35:38Z

    'long dash': array( [ 9.0, 5.0 ] )
 }
-__line_style_trait_map_keys = __line_style_trait_values.keys()
+__line_style_trait_map_keys = list(six.iterkeys(__line_style_trait_values))


For efficiency

jdeschenes · 2017-07-27T01:42:31Z

+    @unittest.skipIf(six.PY3, reason="Crashes on python 3. See GH #95.")
    def test_draw_24(self):
-        gc = GraphicsContext((256, 128), pix_format='rgba32')
+        gc = GraphicsContext((256, 128), pix_format='rgb24')


The pix_format is probably wrong. This is a test_draw_24 and it is using rgba32. We might want to add another test to cover the normal case as well?

jdeschenes · 2017-07-27T01:51:05Z


        if event_type == 'character':
-            key = unicode(event.text())
+            key = six.u(event.text())


jdeschenes · 2017-07-27T01:51:31Z


 import warnings

+import six


jdeschenes · 2017-07-27T01:51:36Z

    def __init__(self, parent, *args, **kwargs):
        wx.Frame.__init__(self, parent, *args, **kwargs)
-        self.pathOps = dict((k,v) for (k,v) in wx.GraphicsPath.__dict__.iteritems() if k.startswith("Add"))
+        self.pathOps = dict((k,v) for (k,v) in six.iteritems(wx.GraphicsPath.__dict__) if k.startswith("Add"))


jdeschenes · 2017-07-27T01:51:58Z

-    except ImportError, SystemExit:
-        raise ImportError, "Unable to import a Savage backend for the %s " \
-            "toolkit." % ETSConfig.toolkit
+    except (ImportError, SystemExit):


Yes, it will raise a SyntaxError otherwise

jdeschenes · 2017-07-27T01:57:43Z

Thanks for the quick review by the way!

* Added a test for the `test_draw_24` * Fixed an import error in cairo.py * Removed changes from tohtml.py * Removed reference of CP Trac #2111 in comments * Fixed issue in function parseAFMFile * Added very preliminary test for parseAFMFile

jdeschenes · 2017-08-02T00:26:38Z

I have changed a few more things from @jwiggins comments. I think all comments except itervalues, iterkeys and iteritems have been answered. If not, please advise. I will make the last change tomorrow.

# Conflicts: # enable/qt4/base_window.py # enable/wx/base_window.py

jdeschenes · 2017-08-02T21:18:30Z

I think I have resolved all issues. Let me know if there is anything else to correct.

jwiggins

It looks like there are only some import cleanups needed now. This is otherwise ready to merge!

jwiggins · 2017-08-03T19:38:13Z

 from __future__ import with_statement

+import six
+


This import is unused here.

jwiggins · 2017-08-03T19:40:49Z

 """

+import six
+


Also unused here

jwiggins · 2017-08-03T19:48:19Z

 from six import StringIO
 import xml.etree.cElementTree as etree

+import six


Unused import

jwiggins · 2017-08-03T19:54:16Z

@@ -1,5 +1,7 @@
 import unittest

+import six


Unused import

jwiggins · 2017-08-03T19:54:50Z

 import copy
 import unittest

+import six


jwiggins · 2017-08-03T20:26:22Z


 import sys, string, struct, re, os.path
-
+import six


Unused import

jwiggins · 2017-08-03T20:26:33Z

 #   marker_enum_map[enum] = string
 #
 #----------------------------------------------------------------------------
+import six


Unused import

jwiggins · 2017-08-03T20:28:44Z

    from traits.api import false
    from chaco.api import ArrayPlotData, Plot, PlotGraphicsContext
    from chaco.example_support import COLOR_PALETTE
+    import six.moves as sm


This was already imported at the top of this module.

jwiggins · 2017-08-03T20:38:10Z

+import tempfile
+from contextlib import contextmanager
+
+from six.moves import StringIO


This import appears to be unused

jwiggins · 2017-08-03T20:38:51Z

 #-------------------------------------------------------------------------------
 #  Imports:
 #-------------------------------------------------------------------------------
+import six


Unused import

* Removed unused imports * added `from __future__ import print_function` in tohtml.py * fixed syntax error in vu_meter.py

jdeschenes · 2017-08-03T21:51:03Z

Fixed that unused imports. There was another issue that went unnoticed in vu_meter that I fixed.

jwiggins · 2017-08-04T07:18:41Z

I think I'm basically done reviewing this. Does anybody with fresh eyes want to take a look before it's merged?

jwiggins · 2017-08-05T20:27:33Z

-        print '  red = ',
+            print(self.stops[i], end=" ")
+        print()
+        print('  red = ', end=" "


This line is missing its closing )

jwiggins · 2017-08-05T20:50:06Z

OK, so I finally had a chance to test this. Looks good, modulo the missing parenthesis which causes a build failure on macOS.

jdeschenes · 2017-08-06T15:34:06Z

I added the parenthesis. Building MacOS on travis should be done quite quickly so this doesn't slip through!

jwiggins · 2017-08-06T18:26:00Z

Thanks for all the updates @jdeschenes!

jmdeschenes and others added 22 commits September 7, 2016 09:55

Python 3 compatibility

f30695d

* print should now be functions instead of statements * raises should now respect the new syntax * "<>" operator was replaced by "!="

Python 3 compatibility

a0b54f2

* Fixed tuple unpacking issues * replaced has_key with "in" syntax * exec is now a function instead of a statement

Python 3 compatibility

c05e2c4

* Fixed __metaclass__ compatibility using six.add_metaclass

Python 3 Compatibility

0287cad

* replaced xrange by six.moves.range

Python 3 compatibility

9838127

* Fixed tuple unpacking issues * Fixed izip issues using six

Python 3 compatibility

1fb7e72

* Replaced self.assert_ with self.assertTrue

Python 3 compatibility

2cd1509

* Fixed several non-trivial list vs iterables in kiva

Python 3 compatibility

67f76f3

* Fixed several non-trivial list vs iterables in enable

Python 3 compatibility

903f48b

* Fixed reduce function using six * Fixed a few other issues not seen previously

Python 3 compatibility

c1f66e0

* fixed unicode and unichr calls

Python 3 compatibility

f950f42

* Fixed StringIO with six

Python 3 compatibility

d43a075

* Fixed imports * Fixed some StringIO that were missed in the initial pass * Some string fixes

Python 3 compatibility

3aebe74

* Fixed issues with basestring * fixed issues with long

Python 3 compatibility

c9e571b

* Some fixes with import *

Python 3 compatibility fixes

9acbd4d

Solved some issues while running unit test.

Fixed issues with python 3 string in font_manager

efbae1e

Extra modifications

6b62549

Forgot to add a import six.

58a690d

Several fixes for py3

2a7983e

All unit tests passes.

fd4fd79

Revisions from comparing master branch.

fc07976

jdeschenes added 2 commits July 24, 2017 21:34

Used proper reason for unittest skip

7bda4f8

Small corrections.

b015584

jdeschenes mentioned this pull request Jul 25, 2017

kiva/fonttools/fontmanager.py:findfont unable to find suitable fonts #259

Closed

jwiggins requested changes Jul 26, 2017

View reviewed changes

From comments of @jwiggins, there were extra parenthesis.

1967c3c

jwiggins requested changes Jul 26, 2017

View reviewed changes

jdeschenes commented Jul 27, 2017

View reviewed changes

jdeschenes added 2 commits August 1, 2017 17:54

Merge branch 'master' into python3_compat

412b479

Changes from @jwiggins comments.

dffcaa5

* Added a test for the `test_draw_24` * Fixed an import error in cairo.py * Removed changes from tohtml.py * Removed reference of CP Trac #2111 in comments * Fixed issue in function parseAFMFile * Added very preliminary test for parseAFMFile

jdeschenes added 3 commits August 2, 2017 17:07

Removed six.itervalues, iterkeys and iteritems where unnecessary

a8e9605

Merge branch 'master' into python3_compat

7afc595

# Conflicts: # enable/qt4/base_window.py # enable/wx/base_window.py

Forgot one iteritems

e3830f0

jwiggins requested changes Aug 3, 2017

View reviewed changes

Another round of comments from @jwiggins

4ea95a5

* Removed unused imports * added `from __future__ import print_function` in tohtml.py * fixed syntax error in vu_meter.py

jwiggins approved these changes Aug 4, 2017

View reviewed changes

jwiggins reviewed Aug 5, 2017

View reviewed changes

Added missing parenthesis.

812b243

jwiggins merged commit 8cb8dd4 into enthought:master Aug 6, 2017

kitchoi mentioned this pull request Dec 14, 2020

Fix AttributeError on Python 3.9 due to usage of removed API in xml #492

Merged

Conversation

jdeschenes commented Jul 25, 2017

Uh oh!

codecov-io commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jdeschenes commented Jul 25, 2017

Uh oh!

jdeschenes commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeschenes commented Jul 26, 2017

Uh oh!

jwiggins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwiggins commented Jul 26, 2017

Uh oh!

jdeschenes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-io commented Jul 25, 2017 •

edited

Loading

jdeschenes commented Jul 25, 2017 •

edited

Loading

jdeschenes left a comment •

edited

Loading