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 #106

Merged
merged 1 commit into from
Jan 25, 2016
Merged

Python 3 compatibility #106

merged 1 commit into from
Jan 25, 2016

Conversation

jmgirven
Copy link

This commit takes the current version and makes it compatible with Python 3 (#85). Nothing too complicated. 2to3 does most of the work.

There are no major changes in apns.py, just syntax and explicitly made some parts of the codes bytes to compatible across 2 and 3.

Similar in tests.py. Only line 191-193 needed a small change to account for different ordered dictionaries. An alternative could be test the beginning of the line, the end of the line, and json parse the middle and compare.

@jimhorng
Copy link
Collaborator

to make code py3 compatible is good, however, it's better to keep py2 function's behavior when running by py2, for example xrange(), if you turn xrange() into range() which becomes a list instead of iterator, that will harm resource consumption and performance.
Maybe you can take reference from cheetsheet
use pip install future

# Python 2 and 3: backward-compatible
from past.builtins import xrange
for i in xrange(10**8):
    ...

@jimhorng jimhorng mentioned this pull request Jul 17, 2015
djacobs added a commit that referenced this pull request Jan 25, 2016
@djacobs djacobs merged commit 4a7334b into djacobs:master Jan 25, 2016
@ExplodingCabbage
Copy link
Collaborator

@jimhorng I don't think the range vs xrange performance difference is worth caring about here. We're talking about sometimes creating a list of 10 integers when you otherwise wouldn't have to. That really, really doesn't matter in the grand scheme of things; it's not worth adding any complexity to the code just to avoid creating a list of 10 ints.

@jimhorng
Copy link
Collaborator

@ExplodingCabbage agreed +1

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