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 support #220

Merged
merged 16 commits into from Mar 11, 2015

Conversation

Projects
None yet
3 participants
@myersjustinc
Copy link
Contributor

myersjustinc commented Mar 11, 2015

Should take care of #77, even if it took a little longer than expected. Works on my installations of 2.7.8 and 3.3.5. (I should get around to updating those.)

Changes:

  • Removed long literals (e.g., max_length=55L), which aren't a thing anymore.
  • Used io.StringIO instead of cStringIO.StringIO, so strings of all flavors can be passed as arguments without a lot of juggling.
  • Allowed Unicode to render properly in yes/no prompt at beginning of download process.
  • Replaced model __unicode__ methods with cross-compatible __str__ equivalents per the Django docs.
  • Used csvkit consistently throughout for Unicode-aware CSV handling.
  • Ensured model/admin __init__ imports worked consistently across versions.

myersjustinc added some commits Mar 10, 2015

Make imports more explicit
These worked fine in Python 2, but they were failing in Python 3. This
resolves that discrepancy.
Remove long integer spec in model definitions
Longs aren't a thing in Python 3.
Update StringIO import for Unicode compatibility
`io.StringIO` is Unicode-compatible, but cStringIO isn't.

The `io` module exists in 2.6+, so we should be OK.
Ensure stdout can handle Unicode data
This is an alternate solution to @aboutaaron's solution to #22, which
worked great under Python 2 but just shows a bytestring `__repr__`
under Python 3. For reasoning, see http://bit.ly/1C3l4eV.
Write download metadata to text file, not binary
We don't actually need bytes here when we're just writing a string.
Create log directory if it doesn't exist
Others get done properly elsewhere, but logs get left out.
Narrow scope of stdout shenanigans
The stdout stuff I did in d0d8565 was
causing problems elsewhere for `CalAccessCommand.log` calls, so this
reverses the change once it's no longer needed.
Switch to csvkit readers and writers in all uses
They're designed to handle all this Unicode/bytes nonsense for us, so
why not take advantage of them?
Fix CSV line ending spec in loader
`CSVKitWriter` was writing these files with different line endings, it
seems, than `csv.writer` was, so these appeared to MySQL as one giant
line that was getting skipped. This resolves that error (and still
works fine in Python 2).
Fix Python 2 encoding issues in cleaner
After I got this working in Python 3, I got some encoding- (and bytes-)
related issues under Python 2 instead. These changes finally make both
sides happy.
Make model `__unicode__` methods compatible
`__unicode__` isn't a standard method in Python 3 since Unicode isn't
a separate type anymore. This commit adjusts those methods to follow
[Django's porting advice](https://docs.djangoproject.com/en/1.7/topics/python3/#str-and-unicode-methods)
and ensures they actually return strings (since some previously
returned integers).
Fix TSV delimiter spec in cleaner
Remember when I said in e6271f5,
"These changes finally make both sides happy"? That was a bunch of
lies--dirty, dirty lies.

For the `delimiter` argument to `csv.reader` (and therefore
`csvkit.CSVKitReader`), Python 2 requires it be a `str` (i.e., a
bytestring), and Python 3 requires it _not_ be a `bytes`, which just
seems silly, and had me bashing my head against a wall trying to figure
out what to do (since `six` wasn't much help), until I realized they
both just wanted whatever they called `str`. Easy enough, once you
think about it, but not trivial with
`from __future__ import unicode_literals` (which, yes, I added in the
aforementioned Commit of Lies, but kept since it _is_ good practice).
Merge branch 'master' into py3-rewrite
Conflicts:
	calaccess_raw/management/commands/cleancalaccessrawfile.py
	calaccess_raw/models/campaign.py
	calaccess_raw/models/other.py
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 11, 2015

Coverage Status

Coverage increased (+0.03%) to 98.22% when pulling 1b77a3a on myersjustinc:py3-rewrite into 17ede3f on california-civic-data-coalition:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 11, 2015

Coverage Status

Coverage increased (+0.03%) to 98.22% when pulling 1b77a3a on myersjustinc:py3-rewrite into 17ede3f on california-civic-data-coalition:master.

Add Python 3 to Travis options
I suppose it _did_ help, and #77 did talk about support _and_ testing.
@palewire

This comment has been minimized.

Copy link
Member

palewire commented Mar 11, 2015

Thanks for sticking with it! I'll download the code and give this a try.

@myersjustinc

This comment has been minimized.

Copy link
Contributor

myersjustinc commented Mar 11, 2015

Glad to do it. I'm noticing on the current Travis build that it's failing on Django 1.4--turns out Django doesn't support Python 3 until 1.5.

Everything else is passing so far (but taking some time), but when it complains that the build failed, that's why. If you're OK with that, I can add another commit to this PR to get rid of the Django 1.4 test.

@palewire

This comment has been minimized.

Copy link
Member

palewire commented Mar 11, 2015

I'm working on that very issue right now. We don't need to support those old versions of Django so I'm going to cut the tests.

@myersjustinc

This comment has been minimized.

Copy link
Contributor

myersjustinc commented Mar 11, 2015

Sounds good—I'll leave it alone, then.

@palewire

This comment has been minimized.

Copy link
Member

palewire commented Mar 11, 2015

In case you are curious, Travis has a "Matrix" setting that lets you omit certain envs for Python 3, ala django-debug-toolbar

@myersjustinc

This comment has been minimized.

Copy link
Contributor

myersjustinc commented Mar 11, 2015

Nice. First time using Travis, actually, so I'm still getting used to such things—but good to know!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 11, 2015

Coverage Status

Coverage increased (+0.03%) to 98.22% when pulling 36b7947 on myersjustinc:py3-rewrite into 17ede3f on california-civic-data-coalition:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 11, 2015

Coverage Status

Coverage increased (+0.03%) to 98.22% when pulling 36b7947 on myersjustinc:py3-rewrite into 17ede3f on california-civic-data-coalition:master.

@palewire palewire merged commit 36b7947 into california-civic-data-coalition:master Mar 11, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@palewire

This comment has been minimized.

Copy link
Member

palewire commented Mar 11, 2015

You're in! Thanks again!

HERO

@myersjustinc

This comment has been minimized.

Copy link
Contributor

myersjustinc commented Mar 11, 2015

You're welcome! Glad to help.

@myersjustinc myersjustinc deleted the myersjustinc:py3-rewrite branch Mar 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment