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 dual support #266

Merged
merged 9 commits into from
Jan 4, 2018
Merged

Python dual support #266

merged 9 commits into from
Jan 4, 2018

Conversation

and3rson
Copy link
Collaborator

@and3rson and3rson commented Jan 2, 2018

  • Manually refactored everything with support of 2to3 output
  • Introduced with_metaclass from six (to avoid adding entire six library as a dependency) and migrated all classes that had __metaclass__ attribute
  • Introduced text_type from six
  • Introduced text_types
  • Introduced xrange
  • Fixed string literals
  • Fixed docstring tests
  • Misc 2to3 fixes (deprecated argument unpacking in function signature, deprecated raise syntax etc.)
  • Imported print_function from __future__
  • Added unicode_literals from __future__ to font.py

Outcome:

  • All tests pass in Python 2.7 and 3.6.
  • Everything imports.
  • My Google Music Player works with this branch in both Python 2 & 3.
  • All examples work.

@wardi I'd like you to confirm that this can be merged. These changes are pretty massive and I want an additional confirmation from you to be extra sure that this can be merged.

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage increased (+0.1%) to 76.009% when pulling eeaaf65 on python-dual-support into ab6bfbc on master.

@urwid urwid deleted a comment from coveralls Jan 2, 2018
@urwid urwid deleted a comment from coveralls Jan 2, 2018
@urwid urwid deleted a comment from coveralls Jan 2, 2018
@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage increased (+0.2%) to 76.072% when pulling eeaaf65 on python-dual-support into ab6bfbc on master.

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage increased (+0.5%) to 76.376% when pulling f50b182 on python-dual-support into ab6bfbc on master.

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.5%) to 76.389% when pulling c4292f0 on python-dual-support into ab6bfbc on master.

@abadger
Copy link
Contributor

abadger commented Jan 3, 2018

Note: I dislike unicode_literals because it causes great confusion when debugging. You can no longer tell what a bare literal is just by looking at the literal. You have to look at the top of the file for a from future import as well as at the literal. (Things get even worse when some code combines a literal from a file with unicode_literals with a literal from a file without unicode_literals).

It is a little verbosity in font.py but I'd use something like this to do it instead:

from urwid.compat import text_type

def to_text(obj, encoding='utf-8', errors='strict'):
    if isinstance(obj, text_type):
        return obj
    elif isinstance(obj, bytes):
        return obj.decode(encoding, errors)
[...]

""", to_text(r"""
 "###$$$%%%'*++,--.///:;==???[[\\\]]^__`
 " ┼┼┌┼┐O /'         /.. _┌─┐┌ \   ┐^  `
   ┼┼└┼┐ /  * ┼  ─  / ., _ ┌┘│  \  │
     └┼┘/ O    ,  ./       . └   \ ┘ ──
""")]

This makes font.py a little bit more verbose but it can make debugging much less confusing in the future.

@abadger
Copy link
Contributor

abadger commented Jan 3, 2018

One thing I don't see in this PR but would also be good if you're going the route of making this a dual py2-py3 code base would be to add "from future import division" everywhere. This makes "/" yield a float and "//" do integer division. urwid does division in a few places where this distinction is important. Using the division future import will make sure that when there's a bug in the division, it shows up equally in both python2 and python3 (hopefully being detected faster).

@and3rson
Copy link
Collaborator Author

and3rson commented Jan 3, 2018

@abadger Good catch! I'm going to include those updates as well.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 76.373% when pulling e4ecb3b on python-dual-support into ab6bfbc on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 76.373% when pulling e4ecb3b on python-dual-support into ab6bfbc on master.

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.5%) to 76.443% when pulling e4ecb3b on python-dual-support into ab6bfbc on master.

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.5%) to 76.439% when pulling e59c06d on python-dual-support into ab6bfbc on master.

@abadger
Copy link
Contributor

abadger commented Jan 3, 2018

This looks good to me!

@and3rson
Copy link
Collaborator Author

and3rson commented Jan 3, 2018

@wardi any objections? Otherwise I'm merging this.

@and3rson and3rson added Feature Feature request/implementation in progress labels Jan 3, 2018
@and3rson and3rson merged commit cf0307c into master Jan 4, 2018
@and3rson and3rson deleted the python-dual-support branch January 4, 2018 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature request/implementation in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants