Skip to content
This repository has been archived by the owner on Jun 1, 2018. It is now read-only.

Refactor HTTP Request Model #100

Merged
merged 4 commits into from
Sep 28, 2015
Merged

Refactor HTTP Request Model #100

merged 4 commits into from
Sep 28, 2015

Conversation

mhils
Copy link
Member

@mhils mhils commented Sep 25, 2015

This PR is a follow-up on #99 and improves our Python 3 unicode story. We split the request class into RequestData, which works quite similar to .fields for Headers and Request, which provides convenient str access. @property for all the attributes is a bit verbose, but it makes it pretty clear how things work. Also, there are proper docs for this on the mitmproxy/http-models branch. Last but not least, we use .content as a default instead of .body again, because that's what requests does.

@cortesi, is this what you had in mind?

TODO:

  • Refactor response
  • Tests
  • Start removing deprecated API (not part of this PR)

@cortesi
Copy link
Member

cortesi commented Sep 26, 2015

Yes, this looks good to me. I don't love the name "data" for the inner attribute, but I also don't have any more explanatory suggestions. :)

@mhils
Copy link
Member Author

mhils commented Sep 26, 2015

Refactored the Response class as well - we're now at 100% branch coverage for netlib.http, ready for review 😃

"""
HTTP request method, e.g. "GET".
"""
return _native(self.data.method)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about also applying .upper() here? It would cleanup the code in a few other places, and if somebody really needs the raw method, it is still there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Kriechi
Copy link
Member

Kriechi commented Sep 28, 2015

LGTM! :shipit:

@mhils mhils merged commit 23d13e4 into master Sep 28, 2015
@Kriechi Kriechi deleted the http-models branch September 29, 2015 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants