Skip to content
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

Clarify that headers are ADDED to the response #2750

Merged
merged 1 commit into from Sep 20, 2016

Conversation

Projects
None yet
4 participants
@mark0978
Copy link
Contributor

commented Aug 17, 2016

Make it clear that headers are added to the response instead of overwriting existing headers. Explain how to set the content_type instead of just assuming they will know.

Clarify that headers are ADDED to the response
Make it clear that headers are added to the response instead of overwriting existing headers.  Explain how to set the content_type instead of just assuming they will know.
@mmerickel

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

LGTM.

I'd also ask whether you know also that the HTTPExceptions automatically support json responses as of 1.7, so you may not need to be doing whatever you're doing if the client claims it accepts json responses.

a list of (k,v) header pairs
a list of (k,v) header pairs or a dict to be added to the
response, use content_type='application/json' kwarg to change
the content type of the response.

This comment has been minimized.

Copy link
@stevepiercy

stevepiercy Aug 17, 2016

Member

This needs punctuation to clarify. Previously it seemed that only a list was acceptable. Now it seems that either a list or a dict is acceptable but that only the dict will be added to the response.

Additionally the passage is a fragment and a run-on, and should be broken into a statement and a sentence.

Suggest something like this:

A list of (k,v) header pairs or a dict. The headers will be added to the response.
Use the kwarg ``content_type='application/json'`` to change
the content type of the response.
@bertjwregeer

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

I believe that Set-Cookie is actually the ONLY header that may be duplicated in an HTTP Response.

Wonder if I should add a fix in WebOb so that when this merge happens it raises an error if you have duplicates other than Set-Cookie.

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

@bertjwregeer Yeah I'm not sure raising an error is the right answer but perhaps the extend logic would collapse certain blacklisted headers that are known to not allow duplicates (or a conjugate approach with a whitelist). That sounds like a can of worms to me though.

@mmerickel mmerickel merged commit ddc0ee3 into Pylons:master Sep 20, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mmerickel added a commit that referenced this pull request Sep 20, 2016

mmerickel added a commit that referenced this pull request Sep 20, 2016

@mmerickel

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

I tweaked this a bit and merged it. Thanks again @mark0978 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.