Let httpx transport errors fall through to caller - #262
Conversation
Pull Request Test Coverage Report for Build 18281444430Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #261 by modifying the HTTPXEndpoint to let httpx transport errors propagate to the caller instead of being caught and handled internally. The change narrows the exception handling from httpx.HTTPError to httpx.HTTPStatusError, allowing transport-level errors like RemoteProtocolError to bubble up.
Key changes:
- Modified exception handling in HTTPXEndpoint to catch only
httpx.HTTPStatusErrorinstead of the broaderhttpx.HTTPError - Added test coverage for transport error scenarios
- Added pytest to optional dependencies to satisfy flake8-requirements
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sgqlc/endpoint/httpx.py | Changed exception handling from HTTPError to HTTPStatusError to allow transport errors to propagate |
| tests/test-endpoint-httpx.py | Added test case and pytest import to verify transport errors bubble up to caller |
| pyproject.toml | Added pytest as optional dependency for flake8-requirements compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
|
|
||
| def test_server_http_transport_error(respx_mock): | ||
| 'Test if a transport error will get passed back to the caller' |
There was a problem hiding this comment.
[nitpick] Function docstring should use triple quotes for multi-line format consistency with other test functions.
| 'Test if a transport error will get passed back to the caller' | |
| """Test if a transport error will get passed back to the caller""" |
|
thanks again, @kewisch! Released as https://pypi.org/project/sgqlc/17.1/ |
Fixes #261
I needed to add pytest to optional depdendencies since flake8-requirements will prefer PEP621 over poetry's groups and apparently PEP 735 is not yet supported. I get
I900 'pytest' not listed as a requirementotherwise.https://github.com/arkq/flake8-requirements/blob/30242fe4b86730b69816e2d4e0ac2b74894b6381/src/flake8_requirements/checker.py#L743
Please see open question in #261 but I wanted to get this off my chest in case this is your preferred solution