Feature: remove ctype params #301

Merged
merged 5 commits into from Dec 10, 2016

Projects

None yet

1 participant

@bertjwregeer
Member
bertjwregeer commented Dec 10, 2016 edited

This is related to #261 and #130, however these still had the following weird side-effects:

Previous

Some cases to demonstrate the changes:

Case 1

res = Response()
res.charset = 'UTF-16'

assert res.headers['Content-Type'] == 'text/html; charset=UTF-16'

# Change the Content-Type
res.content_type = 'text/plain'

assert res.charset == 'UTF-16'
assert res.headers['Content-Type'] == 'text/plain; charset=UTF-16'

Case 2

res = Response()
res.charset = 'UTF-16'

assert res.headers['Content-Type'] == 'text/html; charset=UTF-16'

# Change the Content-Type
res.content_type = 'image/jpeg'

assert res.charset is None
assert res.headers['Content-Type'] == 'image/jpeg'

# Change the Content-Type
res.content_type = 'text/plain'

assert res.charset is None
assert res.headers['Content-Type'] == 'text/plain'

Now we are returning a Content-Type without a charset, which is not ideal at all.

Case 3

res = Response()
res.content_type_params['level'] = 1

assert res.headers['Content-Type'] == 'text/html; charset=UTF-8; level=1'

# Change the Content-Type
res.content_type = 'image/jpeg'

assert res.charset is None
assert res.headers['Content-Type'] == 'image/jpeg; level=1'

Which may be rather surprising to people.

With this patch

All content-type parameters are removed, and only if the content-type is "texty" do we add the charset back. This means users will have to explicitly add back any content-type paramaters.

Case 1

res = Response()
res.charset = 'UTF-16'

assert res.headers['Content-Type'] == 'text/html; charset=UTF-16'

# Change the Content-Type
res.content_type = 'text/plain'

assert res.charset == 'UTF-8'
assert res.headers['Content-Type'] == 'text/plain; charset=UTF-8

All Content-Type parameters are removed, and the default charset is added because the new Content-Type is "texty".

Case 2

res = Response()
res.charset = 'UTF-16'

assert res.headers['Content-Type'] == 'text/html; charset=UTF-16'

# Change the Content-Type
res.content_type = 'image/jpeg'

assert res.charset is None
assert res.headers['Content-Type'] == 'image/jpeg'

# Change the Content-Type
res.content_type = 'text/plain'

assert res.charset == 'UTF-8'
assert res.headers['Content-Type'] == 'text/plain; charset=UTF-8'

This now correctly sets a charset Content-Type parameter to the default charset and we return a response to the client that makes sense.

Case 3

res = Response()
res.content_type_params['level'] = 1

assert res.headers['Content-Type'] == 'text/html; charset=UTF-8; level=1'

# Change the Content-Type
res.content_type = 'image/jpeg'

assert res.charset is None
assert res.headers['Content-Type'] == 'image/jpeg'

This correctly removes all Content-Type parameters, and doesn't set the charset because image/jpeg is not "texty".


This does mean that if you want to explicitly keep the Content-Type parameters, you'll need to copy them and add them back after setting the Content-Type.

res = Response()
params = res.content_type_params
res.content_type = 'some/application'
res.content_type_params = params

This will also reset the charset back to what it was previously.

This is a backwards incompatible change, but one that changes the API to one that has a lot less surprises in it.

bertjwregeer added some commits Dec 10, 2016
@bertjwregeer bertjwregeer Remove all Content-Type params when setting Content-Type
This rips the band-aid off, instead of lingering on and only removing
the charset from the the Content-Type, it just removes all Content-Type
parameters which is more correct than previous.

This also will add back a charset if the Content-Type is "texty", using
the default_charset if it exists.
c422ae4
@bertjwregeer bertjwregeer charset is reset, update test 780912f
@bertjwregeer bertjwregeer Remove unused warning 825a1bc
@bertjwregeer bertjwregeer Update documentation for Content-Type 19bb0e4
@bertjwregeer bertjwregeer Update docs
e89ef1c
@bertjwregeer bertjwregeer merged commit 93fc4bd into master Dec 10, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@bertjwregeer bertjwregeer deleted the feature/remove_ctype_params branch Dec 10, 2016
@bertjwregeer bertjwregeer added this to the 1.7.0 milestone Dec 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment