-
Notifications
You must be signed in to change notification settings - Fork 42
Add support for Python 3.5 #85
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
Conversation
Most changes were minor changes to switch to methods that work on both Python 2 & 3. The major change was a switch to the underlying CSV library. Based on a suggestion from the Django documentation I switched to the python-unicodecsv library, which handles unicode encoding and works properly with both Python 2 & 3, and is capable of writing to Django's HttpResponse, which presents an interface of a binary file-like object. (https://docs.djangoproject.com/en/1.9/howto/outputting-csv/#using-the-python-csv-library)
dev_requirements.txt
Outdated
coverage==3.6 | ||
flake8==2.5.1 | ||
django>=1.5 | ||
unicodecsv==0.14.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably be unicodecsv>=0.14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in 351a114
LGTM. Bringing in python-unicodecsv looks like the right way to deal with this problem. Is this planned for a major or minor version increase? |
Since unicode handling is now done by the unicodecsv library, the function name was a bit misleading
Since it is intended to provide a drop-in replacement for the standard library CSV module, I don't expect we'll have to deal with breaking changes
Since this library is fairly stable and this is the last major item on the "TODO" list, I figured I'd do a 1.0 release. |
I only have theoretical knowledge of unicode handling in Python 3 but the broad strokes here look right. |
While working on other things I tried provisioning an OpenTreeMap VM with this branch by adding a git url to the requirements.txt file. I hit an error but will make another attempt at doing some regression testing in the context of a Python 2.7 app. |
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built a new OpenTreeMap development app server VM replacing the existing django-queryset-csv
line in requirements.txt
with this line:
git+https://github.com/maurizi/django-queryset-csv.git@villoid-feature/python-3#egg=django-queryset-csv
All unit tests pass.
I was able to successfully create exports of multiple datasets, including data with unicode characters.
Builds on top of #84