Skip to content

Change render_to_csv_response to return a StreamingHttpResponse by default. #90

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 2, 2016
Merged

Change render_to_csv_response to return a StreamingHttpResponse by default. #90

merged 1 commit into from
Nov 2, 2016

Conversation

maurizi
Copy link
Contributor

@maurizi maurizi commented Nov 1, 2016 •
edited
Loading

Change render_to_csv_response to return a StreamingHttpResponse instead of a normal HttpResponse.
The old behaviour is available by setting the keyword argument streaming to False.

This is technically a breaking change, as some middlewares do not work with StreamingHttpResponse.

Also drops support for Python 2.6 and Django 1.5, and cleans up the README a bit.

Fixes #86, Fixes #87

@maurizi maurizi assigned hectcastro and unassigned hectcastro Nov 1, 2016
@maurizi maurizi force-pushed the streaming-response branch 2 times, most recently from ff2144c to 1a5c1dd Compare November 1, 2016 23:03
@maurizi maurizi changed the title Change render_to_csv_response to return a StreamingHttpResponse. Change render_to_csv_response to return a StreamingHttpResponse by default. Nov 1, 2016
@maurizi maurizi force-pushed the streaming-response branch 4 times, most recently from fdbd9d3 to 6a023f9 Compare November 1, 2016 23:38
hectcastro
hectcastro reviewed Nov 2, 2016
View reviewed changes
Copy link
Contributor

@hectcastro hectcastro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Will try exercising the code a bit more tomorrow.

djqscsv/djqscsv.py Outdated
else:
response = StreamingHttpResponse(
_iter_csv(queryset, _Echo(), **kwargs),
content_type='text/csv')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider pulling content_type out into a dict assigned to a variable and then unpacking it into HttpResponse() and StreamingHttpResponse() with **.

djqscsv/djqscsv.py
from django.core.exceptions import ValidationError
from django.utils.text import slugify
from django.http import HttpResponse
from django.http import HttpResponse, StreamingHttpResponse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, StreamingHttpResponse is in all of the supported Django versions (even 1.6). We could probably drop support for 1.6 and 1.7 too, given that 1.7 is unsupported as of the end of Q4 2015.

See: https://www.djangoproject.com/download/#supported-versions

Copy link
Contributor Author

@maurizi maurizi Nov 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I dropped 1.5 because it was failing on Travis CI when run with Python 2.6 (not sure why, logs were broken last night).

Since 1.6 and 1.7 are no longer supported, I just dropped those as well, which let me get rid of some code too!

djqscsv/djqscsv.py

# add BOM to support CSVs in MS Excel (for Windows only)
file_obj.write(b'\xef\xbb\xbf')
yield file_obj.write(b'\xef\xbb\xbf')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice minimal change.

README.rst

In addition to the above arguments, ``render_to_csv_response`` takes the following optional keyword argument:

- ``streaming`` - (default: ``True``) A boolean determining whether to use ``StreamingHttpResponse`` instead of the normal ``HttpResponse``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think defaulting this to True is fine, provided that we bump the major version number in the next release.

hectcastro
hectcastro approved these changes Nov 2, 2016
View reviewed changes
Copy link
Contributor

@hectcastro hectcastro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I think the only thing left is to finalize the versions of Django we want to support.

@maurizi maurizi force-pushed the streaming-response branch from 6a023f9 to e2ef154 Compare November 2, 2016 14:29
@maurizi
Copy link
Contributor Author

maurizi commented Nov 2, 2016 •
edited
Loading

@hectcastro Thanks for the review! I think this is ready to go, I'll merge it once all the tests pass.

@maurizi maurizi force-pushed the streaming-response branch from e2ef154 to 919b754 Compare November 2, 2016 14:35
@maurizi
Change render_to_csv_response to return a StreamingHttpResponse.
e98ac8f 
The old behaviour is available by setting the keyword argument
`streaming` to `False`.

Also drop support for Python 2.6 and Django 1.5-1.7, adds Django 1.10 to
the test matrix, and cleans up the README a bit.

Fixes #86, Fixes #87
@maurizi maurizi force-pushed the streaming-response branch from 919b754 to e98ac8f Compare November 2, 2016 15:02
@coveralls
Copy link

coveralls commented Nov 2, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling e98ac8f on streaming-response into 3ab7a2c on master.

@maurizi maurizi merged commit f9412d4 into master Nov 2, 2016
@maurizi maurizi deleted the streaming-response branch November 2, 2016 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@hectcastro hectcastro hectcastro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hectcastro hectcastro

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maurizi @coveralls @hectcastro