feat(py): allow passing binary body to the data parameter - #103
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR widens the accepted type for the data parameter in HTTP requests, enabling binary data to be sent alongside form-encoded data. Key changes include:
- New tests (both sync and async) that verify passing a binary (or JSON string) body via the data parameter.
- Introduction of a new Rust enum (RequestBody) to handle different data types.
- Updating function signatures in multiple modules to accept RequestBody instead of a dictionary.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| impit-python/test/basic_test.py | Added test for passing string body as binary data |
| impit-python/test/async_test.py | Added async test for passing binary body via data |
| impit-python/src/request.rs | Introduced RequestBody enum and form_to_bytes helper function |
| impit-python/src/lib.rs | Updated impit function signature to accept RequestBody for data |
| impit-python/src/client.rs | Updated Client methods to accept RequestBody for data |
| impit-python/src/async_client.rs | Updated AsyncClient methods to accept RequestBody for data |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the API to allow passing a binary body to the data parameter while maintaining support for form data. Key changes include:
- Adding new tests (both synchronous and asynchronous) for binary and non-ASCII input.
- Introducing a new RequestBody enum in Rust with variants for bytes, form, and catch‐all.
- Updating function signatures and internal request handling logic in multiple modules to use RequestBody.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| impit-python/test/basic_test.py | Added tests for binary request body and form data with non-ASCII. |
| impit-python/test/async_test.py | Added async tests for binary body and non-ASCII form data. |
| impit-python/src/request.rs | Introduced RequestBody enum and form_to_bytes helper function. |
| impit-python/src/lib.rs | Updated function signature to accept RequestBody instead of HashMap. |
| impit-python/src/client.rs | Revised request functions to handle RequestBody and convert form data. |
| impit-python/src/async_client.rs | Revised async request functions to handle RequestBody consistently. |
| impit-python/Cargo.toml | Added dependency for urlencoding. |
Comments suppressed due to low confidence (1)
impit-python/test/basic_test.py:90
- [nitpick] The trailing underscore in 'test_form_non_ascii_' may be confusing; consider renaming the function to 'test_form_non_ascii' for clarity.
def test_form_non_ascii_(self, browser: Browser) -> None:
| form_to_bytes(form) | ||
| } | ||
| RequestBody::CatchAll(e) => { | ||
| panic!("Unsupported data type in request body: {:#?}", e) |
There was a problem hiding this comment.
Using panic on an unsupported data type may lead to abrupt crashes in production; consider returning an error instead to allow for graceful handling.
| panic!("Unsupported data type in request body: {:#?}", e) | |
| return Err(format!("Unsupported data type in request body: {:#?}", e)) |
b238a9f to
e9cfa0f
Compare
Following the discussion under #97 , this PR widens the type accepted by the
dataparameter.datanow accepts both adict[str,str]and a binary representation of the request body. This PR intentionally doesn't update the recently added typings in theimpit.pyifile, as this behaviour (accepting the binary body) is considered deprecated in thehttpxlibrary we use as the design master.