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 BaseHTTPResponse a base class of HTTP2Response #3311
Conversation
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.
Only one suggestion, otherwise LGTM!
src/urllib3/connectionpool.py
Outdated
@@ -556,7 +556,7 @@ def _make_request( | |||
# HTTP version | |||
http_version, | |||
response.status, | |||
response.length_remaining, # type: ignore[attr-defined] | |||
getattr(response, "length_remaining", None), |
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.
Can we add support for length_remaining
in this PR?
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.
Do you mean adding self.length_remaining = 0
or actually handling it correctly?
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.
self.length_remaining = 0
seems appropriate, the actual implementation can happen in the streaming issue.
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.
Done. By the way, If we keep length_remaining
eventually, this means we should probably add it to BaseHTTPResponse
and fix the mypy error in the process.
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.
Agreed should be a part of BaseHTTPResponse
.
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.
Recorded here: #3315
Also looks like we need to add a test for |
Closes #3297
As mentioned in #3297, it turns out we already had
version
, an integer that tells us the HTTP version using a convention fromhttp.client
(10 means HTTP/1.0, 11 means HTTP/1.1 and 20 means HTTP/2.0). I'm happy to add anhttp_version
string field too.This pull request was sponsored by Elastic, my employer.