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

HeaderReader class to replace read_headers #25

Merged
merged 9 commits into from Apr 29, 2017

Conversation

Projects
None yet
2 participants
@jaraco
Member

jaraco commented Apr 23, 2017

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.

@webknjaz

Overall it looks okay, except for a few nitpicks.

return hdict
def _allow_header(self, key_name):
return key_name

This comment has been minimized.

@webknjaz

webknjaz Apr 23, 2017

Member

return True?

This comment has been minimized.

@jaraco

jaraco Apr 23, 2017

Member

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.

k = self._transform_key(k)
hname = k
if not self._allow_header(k):

This comment has been minimized.

@webknjaz

webknjaz Apr 23, 2017

Member

It looks you've lost one level of indentation starting here till line 183.

This comment has been minimized.

@webknjaz

webknjaz Apr 23, 2017

Member

Also, what if one wants to run a check before transformation?

This comment has been minimized.

@jaraco

jaraco Apr 23, 2017

Member

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?

This comment has been minimized.

@jaraco

jaraco Apr 23, 2017

Member

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.

This comment has been minimized.

@webknjaz

webknjaz Apr 23, 2017

Member

@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?

This comment has been minimized.

@jaraco

jaraco Apr 23, 2017

Member

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.

This comment has been minimized.

@webknjaz

webknjaz Apr 24, 2017

Member

got it, thanks :)

@@ -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):

This comment has been minimized.

@webknjaz

webknjaz Apr 23, 2017

Member

Please add a docstring here.

raise ValueError('Illegal header line.')
v = v.strip()
k = self._transform_key(k)
hname = k

This comment has been minimized.

@webknjaz

webknjaz Apr 23, 2017

Member

why not assign hname = self._transform_key(k) and use just this var name? do we need two vars for the same purpose?

This comment has been minimized.

@jaraco

jaraco Apr 23, 2017

Member

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.

@jaraco jaraco merged commit a273a93 into master Apr 29, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment