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

Client.headers() now returns a dict, with the key being the response header and the value being a list of response values. #14

Closed
wants to merge 1 commit into from

Conversation

Rolandde
Copy link

You changed the Client.headers() method today in response to my bug report.

You changed it to return a list of tupples (originally returns a dict of strings). I think it might be better to continue returning a dict, as users generally want to isolate specific headers. I rewrote it so that it returns a dict, with the value being a list. I think it is easier for the user to deal with accessing list values than iterating over the list, testing for the equality of the first element to their desired response header.

header and the value being a list of response values.
@niklasb
Copy link
Owner

niklasb commented Oct 18, 2014

This is on purpose, because a dict does not appropriately model HTTP headers. In particular, according to the HTTP specs, there can be multiple instances of the same header, e.g. Set-Cookie.

In case you prefer the old behavior, just use dict(Client.headers()) instead.

@niklasb niklasb closed this Oct 18, 2014
@niklasb
Copy link
Owner

niklasb commented Oct 18, 2014

Oh I just see that your change actually also proposes different semantics that resolve the old issue. I will think about it some more.

@niklasb niklasb reopened this Oct 18, 2014
@Rolandde
Copy link
Author

Using dict(Client.headers()) with the current code, which returns a list of tupples, would re-introduce a bug. It would arbitrarily remove instances of multiple headers. As an example:

l = [('a', 'a1'), ('b', 'b1'), ('a', 'a2')]
d = dict(l)
d
>>> {'a': 'a1', 'b': 'b1'}

@niklasb
Copy link
Owner

niklasb commented Oct 18, 2014

That was just a quick suggestion for when you want the old behaviour back,
when there is only one instance of each header (e.g.
dict(sess.headers())['content-type']). This covers 99,9% of cases. I will
slightly change the semantics so that the keys are normalized to lower-case.

When you want to get your suggested list, you can do [y for x,y in sess.headers().items() if x == 'set-cookie']. That's a bit of boilerplate,
but it does not seem to be a use case common enough to optimize for. I
think it's better to keep it simple, what do you think?

On Sat, Oct 18, 2014 at 5:08 PM, Rolandde notifications@github.com wrote:

Using dict(Client.headers()) with the current code, which returns a list
of tupples, would re-introduce a bug. It would arbitrarily remove instances
of multiple headers. As an example:

l = [('a', 'a1'), ('b', 'b1'), ('a', 'a2')]d = dict(l)d>>> {'a': 'a1', 'b': 'b1'}


Reply to this email directly or view it on GitHub
#14 (comment).

@Rolandde
Copy link
Author

Here are my thoughts:

I will slightly change the semantics so that the keys are normalized to lower-case.

Please don't do that. HTML header strings are standardized (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html) and making them lower-case breaks that standard. I always expect the header string to be 'User-Agent', never 'user-agent'.

Your suggestion of using list comprehension will certainly work. I still like the dictionary approach as I feel it is more useful and consistent. Useful in the sense that most times I call headers() is because I want to find a specific header. The value being a list makes that every so slightly more annoying, but it makes it very explicit that multiple headers of the same name could be returned. Both changes will break code from previous versions, but I feel the dictionary approach is easier to re-factor. Knowing that dict(sess.headers())['Content-Type']`) may drop elements is far more obscure and bugs caused by this will be trickier to track down.

All the best.

@niklasb
Copy link
Owner

niklasb commented Jul 28, 2015

Thanks for the fruitful discussion. I'm closing this because I feel like the current version has quite reasonable behavior in this regard. Header names are now normalized to per-segment capitalized form (i.e. User-Agent).

@niklasb niklasb closed this Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants