feat: Unify the request pipeline across HTTP clients - #1022
Conversation
github-actions
Bot
added
t-tooling
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1022 +/- ##
==========================================
+ Coverage 94.87% 94.93% +0.05%
==========================================
Files 58 58
Lines 5384 5406 +22
==========================================
+ Hits 5108 5132 +24
+ Misses 276 274 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Pijukatel
left a comment
Pijukatel
left a comment
There was a problem hiding this comment.
I see your PR attracted some of my comments to pre-existing code. Well, that is what you get for moving the code around :-D
|
|
||
| @staticmethod | ||
| def _is_body_worth_compressing(data: str | bytes | bytearray | None) -> bool: | ||
| """Whether this body clears the size threshold `_prepare_request_call` compresses at, cheaply. |
There was a problem hiding this comment.
I had to read the first sentence several times, and I am still not sure I understand it correctly. Please rephrase it.
| length decides, and the body is then under 4 KiB, so encoding it here is cheap. | ||
| """ | ||
| if isinstance(data, str): | ||
| return len(data) >= MIN_COMPRESSION_SIZE or len(data.encode('utf-8')) >= MIN_COMPRESSION_SIZE |
There was a problem hiding this comment.
the body is then under 4 KiB, so encoding it here is cheap.
I find it funny that body under 4KiB is too expensive for compression, but cheap enough for pointless encoding call :-)
Do we need such a strict boundary check here since it is only about optimization and not some functional requirements?
| ) | ||
|
|
||
|
|
||
| # -- Protocol conformance tests -- |
There was a problem hiding this comment.
I saw these comments were already in the file, but I am not a big fan of grouping the tests by free-floating comments. There is nothing enforcing the comments. I think that other methods of grouping tests are better, for example: standalone files, pytest markers, group by names, group into class...
3 participants
Moves
call, the retry loop, and the per-attempt request handling fromImpitHttpClientandImpitHttpClientAsyncintoHttpClientandHttpClientAsync, leaving Impit as a thin adapter over the transport hooks:send_request,is_retryable_transport_error,is_timeout_error, andclose()/aclose()plus context managers. A custom client that overridescallkeeps working unchanged and opts out of the shared pipeline.StreamedLogclassifies timeouts through the transport-neutralis_timeout_errorhook instead of importing Impit.No behavior change for the built-in clients: the retry policy shipped in #1019 (permanent transport errors fail fast) and #1020 (a failed read of a streamed error body is classified like a failed send and the response is closed) is preserved — the classification now lives in the
is_retryable_transport_errorhook, and the error-body read handling in the shared pipeline.Supersedes #1011, rebased onto current master and squashed. Originally split out of #1006.
Stack: #1022 (this) → #1004 (HTTPX client) → #1013 (docs).
✍️ Drafted by Claude Code