Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Don't incorrectly default charset on FileResponse #1251

Merged
merged 3 commits into from May 17, 2014

Conversation

Projects
None yet
3 participants
Contributor

dobesv commented Feb 28, 2014

By passing the content_type into the constructor to Response, we can allow it to decide intelligently whether the default charset should apply. Otherwise we'd have to replicate that logic somehow, or live with weird charset annotations on images, pdfs, and zips.

@dobesv dobesv Don't incorrectly default charset on FileResponse
By passing the content_type into the constructor to Response, we can allow it to decide intelligently whether the default charset should apply.  Otherwise we'd have to replicate that logic somehow, or live with weird charset annotations on images, pdfs, and zips.
592fd71
Owner

mmerickel commented Mar 7, 2014

I'm not sure on the rationale behind adding this as a function versus just moving the lines up a bit and passing the options into __init__ but either way it needs to follow the 79-character limit and come with some tests demonstrating the problem it is solving.

Contributor

dobesv commented Mar 7, 2014

I think my mind was affected by java where you cannot have any code before the super call. Sorry about that.

I'll clean it up and add a unit test sometime.

Dobes Vandermeer added some commits Mar 7, 2014

Contributor

dobesv commented Mar 7, 2014

OK I made the changes you requested, please let me know if there's anything more to do.

Owner

bertjwregeer commented May 6, 2014

👍 LGTM.

@mmerickel mmerickel merged commit 06c127e into Pylons:1.5-branch May 17, 2014

1 check passed

default The Travis CI build passed
Details
Owner

mmerickel commented May 17, 2014

Great stuff, I fixed up some line endings and a resource warning. Thanks for the PR!

Owner

mmerickel commented May 17, 2014

I've updated the changelog and forward-ported this onto master as well.

@mmerickel mmerickel added this to the 1.6 milestone Feb 6, 2015

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