HeaderReader class to replace read_headers #25
Conversation
cheroot/server.py
Outdated
| return hdict | ||
|
|
||
| def _allow_header(self, key_name): | ||
| return key_name |
There was a problem hiding this comment.
Hmm. Perhaps. Unless we want to filter out empty headers. I guess the previous implementation allowed for empty headers, so this refactor should as well.
cheroot/server.py
Outdated
| k = self._transform_key(k) | ||
| hname = k | ||
|
|
||
| if not self._allow_header(k): |
There was a problem hiding this comment.
It looks you've lost one level of indentation starting here till line 183.
There was a problem hiding this comment.
Also, what if one wants to run a check before transformation?
There was a problem hiding this comment.
If I add a level of indentation, that would put all of that code within the else block. That code wasn't in the else block before. Why should it be now?
There was a problem hiding this comment.
If one wants to run a check before the transformation, they will either need to override _transform_key to perform the check there, or they should describe why that feature would be useful and add an appropriate hook to enable it.
There was a problem hiding this comment.
@jaraco oh.. I assumed it was.
k variable is created in else-block, meaning we'll get NameError when if part happens, while referencing a variable before assignment. Am I overlooking smth?
There was a problem hiding this comment.
Yes, the code is complex. The other part of the else block (the if block) is indicating a 'continuation line' which is assuming k was set in a previous line (previous iteration of the while loop). So yes, you would get a name error if the first line contained a space or tab, but apparently that's not an issue in practice, as the original code used the same logic.
| @@ -131,53 +131,71 @@ def bton(b, encoding='ISO-8859-1'): | |||
| logging.statistics = {} | |||
|
|
|||
|
|
|||
| def read_headers(rfile, hdict=None): | |||
| """Read headers from the given stream into the given header dict. | |||
| class HeaderReader(object): | |||
| raise ValueError('Illegal header line.') | ||
| v = v.strip() | ||
| k = self._transform_key(k) | ||
| hname = k |
There was a problem hiding this comment.
why not assign hname = self._transform_key(k) and use just this var name? do we need two vars for the same purpose?
There was a problem hiding this comment.
Maybe, although in a loopy/branchy code like this, I've decided to limit my changes to the refactoring in order to avoid potential issues with other optimizations. Since the previous code set k and hname, I decided to retain that. We can optimize that away later if appropriate.
Based on #18 and addressing #17, this approach presents an mechanism by which headers may be filtered or otherwise manipulated as read from the request.