Pull requests

#2 Merged
Repository
t2y
Branch
trunk
Repository
jab
Branch
trunk

Added Python 3 support and dropped under Python 2.5 support

Author
  1. Tetsuya Morimoto
Reviewers
Description

Could you review my changes and merge it to use with Python 3?

I confirmed 3 version is passed with doctest as below.

$ python2.6 minimock.py $ python2.7 minimock.py $ python3.2 minimock.py

  • Learn about pull requests

Comments (5)

  1. jab repo owner

    Thanks for the patch, and sorry I haven't had time to review yet. It probably won't be until next week until I'm freed up. In the meantime, if you'd like to take a peek at the relevant parts of Armin Ronacher's work/writings on py3k porting, and update the pull request accordingly (if there are any changes you'd like to make after reading), it could speed up this patch getting accepted:

    https://github.com/mitsuhiko/python-modernize http://lucumr.pocoo.org/2010/2/11/porting-to-python-3-a-guide/ and from http://lucumr.pocoo.org/2011/1/22/forwards-compatible-python/ :

    Regarding the print-as-a-function future import, I recommend against using it to avoid confusion. Especially because all editors are currently highlighting it as a keyword it can become confusing quickly. Generally if things behave differently in different files it's a good idea to avoid these things if possible. The great aspect of the print change is that it can be reliably converted with 2to3, so there is really no reason to use the print_function future import.

  2. Tetsuya Morimoto author

    Thanks for reply and good information.

    I changed to avoid using print-as-a-function by writing directly. I don't know which is better for maintainability, but it works for me with 3 version's doctest.

  3. jab repo owner

    Just had a chance to review. Could you please make the following changes before I apply?

    • separate the single try/except wrapping the builtin and StringIO imports into two different try/excepts
    • import the newer spelling of each and fall back on the older one instead of vice versa
    • don't drop trying the cStringIO import before falling back on the StringIO import

    In testing this with non-ascii characters, I noticed an inconsistency in the TraceTracker behavior across python2 and python3 due to python3's changes in string printing behavior (example). For instance, if you apply this patch, the tests fail in python2 but pass in python3. Would you like to expand your patch to include better doctest coverage demonstrating expected usage and fixes for additional issues discovered?

    Thanks again for contributing, it's awesome to have you!

    1. Tetsuya Morimoto author
      * separate the single try/except wrapping the builtin and StringIO imports into two different try/excepts
      * import the newer spelling of each and fall back on the older one instead of vice versa
      * don't drop trying the cStringIO import before falling back on the StringIO import
      

      I had misunderstood that it's imported the cStringIO module inside of StringIO, so I changed. Though I gathered importing modules in try/expect syntax, dividing try/expect is also OK. Additionally, it's depend on you (maintainer) which we use new spelling or old one.

      About the difference of repr() is really annoying. :(

      Python 3 repr() behavior is user-friendly, but to implement the same behavior in Python 2.x is needed some complex changes since it should be converted between encoded string and unicode. Instead, I tried to use ascii() for compatibility, I couldn't get a simple solution.

      http://docs.python.org/library/future_builtins.html#future_builtins.ascii

      1. jab repo owner

        Pull request accepted. I reorganized the imports to try the newer spellings before the older ones in 5a3ce7bda056 as well as defining a next() global to unbreak python2.5.

        Re repr, I guess everything's actually fine in the Minimock code itself regardless of Python 2 or 3. You'd only hit this when writing a test with non-ascii characters that has to pass in both Python 2 and 3, so I think it's ok to leave this up to the user.

        Thank you for contributing!