Skip to content
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 Fixes #374

Merged
merged 21 commits into from
Dec 18, 2018
Merged

Conversation

jdeschenes
Copy link
Contributor

As per my issue in the [chaco issue]enthought/chaco#368)

This is a PR to completely get rid of the 2to3 using the six library

Most of the fixes are pretty mechanical and should be pretty straightforward to review:

  • print statement to print function

  • new raise syntax

  • relative imports

  • whenever I could, I converted the list iteration to an iterator

    • items(), keys(), values() to six.iteritems(), iterkeys() and itervalues() (Some of the loops are playing with
      the list, dictionary in the loop and those have been explicitely converted)
    • range to xrange
  • LONG_TYPE instead of long

  • six.u instead of u''

  • and a lot more

The branch has been tested on python 2.7 and python 3.6.

jmdeschenes added 9 commits September 8, 2016 14:29
* fixed several issues from the unit tests.
* Fixed all unit test in traits test except the TestABC...
# Conflicts:
#	docs/source/conf.py
#	traits/etsconfig/etsconfig.py
#	traits/tests/test_get_traits.py
#	traits/tests/test_traits.py
#	traits/trait_notifiers.py
# Conflicts:
#	docs/source/conf.py
#	traits/etsconfig/etsconfig.py
#	traits/tests/test_get_traits.py
#	traits/tests/test_traits.py
#	traits/trait_notifiers.py
@jwiggins
Copy link
Member

Is six.u really necessary if the supported Python versions are 2.7 and 3.4+?

@mdickinson
Copy link
Member

Many thanks for this! This is very valuable, and I hope we can integrate it soon. I've taken a look through the first half of the diff, and left some comments; I'll try to find time to complete the review soon.

Some general comments so far:

  • I think there are a lot of cases where we don't need to use six: in particular, code like for k, v in some_dict.items() can probably be left unchanged.
  • Where a print statment is changed from print a, b to print(a, b), that changes the meaning on Python 2: the new code prints a tuple (with parentheses and commas, and repr-formatted items) where the old code printed space-separated str-formatted items. Those cases probably need to be replaced with string formatting. Example:
>>> import datetime
>>> time_now = datetime.datetime.utcnow()
>>> print "Time now is:", time_now   # old behaviour on Python 2
Time now is: 2017-07-21 08:08:19.598678
>>> print("Time now is:", time_now)  # new behaviour on Python 2
('Time now is:', datetime.datetime(2017, 7, 21, 8, 8, 19, 598678))
>>> print("Time now is: {}".format(time_now))  # proposed replacement
Time now is: 2017-07-21 08:08:19.598678
  • As @jwiggins says: we probably don't need six.u, since u-prefixes work in Python 3.3+, and we're not planning to support Python 3.2. (And probably not Python 3.3, either.)

@mdickinson
Copy link
Member

Those cases probably need to be replaced with string formatting

Using from __future__ import print_function would probably be simpler.

* Added `from __future__ import print_function` to all modules that had
  print statements
* Removed `six.u` in favour of u. Python 3.2 will not be supported
* Fixed a small issue with sorted in has_dynamic_views.py
* Removed a trailing backslash in etsconfig.py
* Removed `six.exec_` in favour of `exec`
* Fixed an issue with a print statement with a trailing comma
@jdeschenes
Copy link
Contributor Author

Hi,
Some changes:

  • I had completelely forgotten about the print function difference in python 2.7. It's now fixed.
  • I removed all six.u references
  • I made some other very small changes

For the items() vs iteritems(), I went over all the cases and changed it manually wherever possible to the iteritems() variant as it is more efficient. It also has the bigger benefit of having the same behavior in both python 2 and 3. I would like to keep it in there, but I can understand that this is a functional change. Would you be open to have this maybe in a separate PR?

* Removed all unused `import six`
* Removed sm.xrange in adaptation benchmark
* Replace six.iteritems mentionned in @mdickinson review
@mdickinson
Copy link
Member

Here's the bug that appears to account for most of the test failures:

(geophysics)bash-3.2$ python
Enthought Deployment Manager -- https://www.enthought.com
Python 2.7.13 |Enthought, Inc. (x86_64)| (default, Mar  2 2017, 08:20:50) 
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from traits.api import *
>>> class A(HasTraits):
...     my_dir = Directory
... 
>>> a = A()
>>> import os                         
>>> a.my_dir = os.path.expanduser("~")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Desktop/traits/traits/trait_handlers.py", line 177, in error
    value )
traits.trait_errors.TraitError: The 'my_dir' trait of an A instance must be a directory name, but a value of '/Users/mdickinson' <type 'str'> was specified.

@mdickinson
Copy link
Member

It looks as though the uses of six.string_types need re-examining. I've left individual comments on all the instances I found that need changing, but I may have missed some.

It's a little odd that six.string_types is always a tuple of length 1; I wonder why Benjamin didn't go with a simple type instead. Perhaps for consistency with six.integer_types?

* fix trait_added property
* fast_validate was not active for `File` trait
* Added unit tests to ensure that the behavior is correct
* Fixed an issue with `Directory` trait giving wrong error when the exist flag is set and type is wrong
@jdeschenes
Copy link
Contributor Author

I fixed the issues you mentionned. I have taken the liberty to add a few test cases to test what was not covered. In doing so, I found a bug with the Directory trait. I fixed it as well.

@jdeschenes
Copy link
Contributor Author

I should also mention that you found all the issues of string_types. So there shouldn't be any more of these problems.

@mdickinson
Copy link
Member

Apologies for the long silence on this; "real" work got in the way, unfortunately.

I've merged master into this branch (some conflicts, mostly trivial; the only significant conflict was in get_defs in has_traits.py. I'm intending to get this finished and merged this week.

@codecov-io
Copy link

Codecov Report

Merging #374 into master will decrease coverage by 2.21%.
The diff coverage is 59.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   66.38%   64.16%   -2.22%     
==========================================
  Files          48       48              
  Lines        7372     7373       +1     
  Branches     1469     1472       +3     
==========================================
- Hits         4894     4731     -163     
- Misses       2076     2203     +127     
- Partials      402      439      +37
Impacted Files Coverage Δ
traits/util/toposort.py 0% <ø> (ø) ⬆️
traits/etsconfig/etsconfig.py 63.58% <0%> (+6.17%) ⬆️
traits/ustr_trait.py 0% <0%> (ø) ⬆️
traits/has_dynamic_views.py 0% <0%> (ø) ⬆️
traits/util/clean_strings.py 0% <0%> (ø) ⬆️
traits/trait_numeric.py 51.97% <0%> (ø) ⬆️
traits/etsconfig/api.py 100% <100%> (ø) ⬆️
traits/__init__.py 85.71% <100%> (+1.09%) ⬆️
traits/_py2to3.py 42.18% <100%> (+1.86%) ⬆️
traits/util/event_tracer.py 96.96% <100%> (+0.11%) ⬆️
... and 18 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 01bf50e...cdabe5a. Read the comment docs.

@mdickinson
Copy link
Member

Merging. @jdeschenes again, apologies for taking so long to get back to this, and enormous thank you for doing this work.

@mdickinson mdickinson merged commit 6208f63 into enthought:master Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants