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
Make header parsing more RFC-7230-compliant #1318
Make header parsing more RFC-7230-compliant #1318
Conversation
urllib3/_collections.py
Outdated
@@ -305,13 +306,22 @@ def from_httplib(cls, message): # Python 2 | |||
# python2.7 does not expose a proper API for exporting multiheaders | |||
# efficiently. This function re-reads raw lines from the message | |||
# object and extracts the multiheaders properly. | |||
obs_fold_continued_leaders = (' ', '\t',) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: trailing comma isn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not needed; I've picked it up as a stylistic thing, though, with tuples; they require a trailing comma to build a 1-tuple, so I've just been doing it consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure whether this consistency is a hobgoblin of mine or not, so opinions on that topic are quite welcome. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just the only thing that stood out to me in this code. :) I think it looks great either way. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that it's not the style of the project, I'd rather we either introduce a check for this to ensure it's consistent across the project or drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. Given that I don't want to blow up the scope of this PR, I'll remove the trailing comma for now and revisit the style later on.
Codecov Report
@@ Coverage Diff @@
## master #1318 +/- ##
=========================================
+ Coverage 97.33% 100% +2.66%
=========================================
Files 21 21
Lines 1989 1991 +2
=========================================
+ Hits 1936 1991 +55
+ Misses 53 0 -53
Continue to review full report at Codecov.
|
@sigmavirus24, hoping to get a review from you on this to make sure it passes RFC muster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit but otherwise this LGTM afaict
…standardization Make header parsing more RFC-7230-compliant
Fixes #1286.
This change ensures that we fail with a descriptive exception in cases where invalid headers beginning with optional whitespace are passed without a prior header to which they can be attached.
This change also brings our parsing of valid line-folded headers more in line with RFC-7230 by joining such a value to the existing header value with a single space character and stripping optional whitespace from either end.