feat: switch to async #2
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR switches the EPR client from synchronous to asynchronous operations. The main purpose is to modernize the HTTP client implementation by replacing urllib3 with httpx and making all client operations async-compatible, which is especially beneficial for MCP servers and other async contexts.
Key changes:
- Replace urllib3 with httpx for HTTP client functionality
- Convert Client class to use async/await pattern with context manager support
- Update GraphQLQuery model to use dict instead of string for variables
- Modify search and create functions to be async and use asyncio.run in CLI
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/epr/client.py | Complete async refactor with httpx integration and context manager support |
| src/epr/models.py | Update GraphQLQuery variables type from string to dict |
| src/epr/create.py | Convert to async function with proper client usage |
| src/epr/search.py | Convert to async function with proper client usage |
| src/epr/main.py | Add asyncio.run calls and fix field handling |
| pyproject.toml | Replace urllib3 dependency with httpx |
| tests/unit/test_errors.py | Update mock import and fix fake exception handling |
| tests/unit/test_models.py | Add comprehensive tests for updated models |
| tests/unit/test_async_client.py | New comprehensive async client tests |
| tests/unit/test_async_search_create.py | New tests for async search/create functions |
| tests/unit/test_async_main.py | New tests for async main functionality |
| tests/test_smoke.py | Add async functionality smoke tests |
| examples/async_client_example.py | New example demonstrating async client usage |
| docs/README.md | Update documentation with async examples |
| README.md | Minor path update for virtual environment |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return [] | ||
| if "," in value: |
There was a problem hiding this comment.
The variable fields is declared but removed in the first condition. This creates inconsistent behavior - the function should either consistently return a list or handle the None case differently to maintain the original variable pattern.
| return [] | |
| if "," in value: | |
| fields = [] | |
| elif "," in value: |
| tb = FakeTraceback([frame], [1]) | ||
| exc = FakeException("foo").with_traceback(tb) | ||
| return FakeException, exc, tb | ||
| return FakeException, exc, exc._fake_traceback |
There was a problem hiding this comment.
Direct access to private attribute _fake_traceback breaks encapsulation. Consider adding a public property or method to access the fake traceback.
| def run_async_test(coro): | ||
| """Helper to run async test methods.""" | ||
|
|
||
| def wrapper(self): | ||
| return asyncio.run(coro(self)) | ||
|
|
||
| return wrapper | ||
|
|
||
|
|
||
| # Apply the async wrapper to all async test methods | ||
| for attr_name in dir(AsyncSearchCreateTestCase): | ||
| attr = getattr(AsyncSearchCreateTestCase, attr_name) | ||
| if callable(attr) and asyncio.iscoroutinefunction(attr) and attr_name.startswith("test_"): | ||
| setattr(AsyncSearchCreateTestCase, attr_name, run_async_test(attr)) |
There was a problem hiding this comment.
The dynamic method wrapping approach is complex and could be error-prone. Consider using unittest.IsolatedAsyncioTestCase (available in Python 3.8+) which provides native async test support, or use pytest with pytest-asyncio for cleaner async test handling.
| def run_async_test(coro): | ||
| """Helper to run async test methods.""" | ||
|
|
||
| def wrapper(self): | ||
| return asyncio.run(coro(self)) | ||
|
|
||
| return wrapper | ||
|
|
||
|
|
||
| # Apply the async wrapper to all async test methods | ||
| for attr_name in dir(AsyncClientTestCase): | ||
| attr = getattr(AsyncClientTestCase, attr_name) | ||
| if callable(attr) and asyncio.iscoroutinefunction(attr) and attr_name.startswith("test_"): | ||
| setattr(AsyncClientTestCase, attr_name, run_async_test(attr)) |
There was a problem hiding this comment.
The dynamic method wrapping approach is complex and could be error-prone. Consider using unittest.IsolatedAsyncioTestCase (available in Python 3.8+) which provides native async test support, or use pytest with pytest-asyncio for cleaner async test handling.
No description provided.