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

Add Python 3 compatibility (updated) #19

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

asavoy
Copy link

@asavoy asavoy commented Jan 2, 2017

This pull request builds from the work by goanpeca in #13.

In addition to the Python 3 compatibility and PEP8 code style fixes in the original branch, these have also been done:

  • Rebased the branch on top of latest master, resolved conflicts
  • Inspected code to ensure latest master changes are present
  • Spot-fixed a case of Python 3 incompatibility
  • Ran test/test_everything.py successfully on Python 2 and Python 3
  • Ran in a Python 3 virtualenv with a live GA account and confirmed that data sent appears in GA
  • Reverted one change to handling VERSION in setup.py, so that it behaves as it does in master

What hasn't been done:

  • I haven't done any deep review of the Python 3 changes. There are some issues that would warrant a closer look, but don't seem to cause any problems in testing:

    • Some instances of str still remain when they probably should be six.text_type
    • Some commented out code
  • I haven't done any manual testing in Python 2 to confirm it works with a live GA account.

I think this is at least at a working point that is worth checking in for a maintainer's feedback, if at least to gauge interest in getting this merged in.

@BoboTiG
Copy link

BoboTiG commented Mar 2, 2018

Any news from the owner?

@olymk2
Copy link

olymk2 commented Sep 19, 2018

can this be merged / can another dev take ownership if it no longer maintained ?

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