Refactor Endpoint Mixins for Modularity and Type Safety #644
Conversation
- Decoupled `ListEndpointMixin` and related mixins by introducing `EndpointProtocol`. - Extracted request preparation logic in `ListEndpointMixin` to `_prepare_list_request`. - Updated `PathGetEndpointMixin` to explicitly handle async requests via `is_async` parameter. - Updated `JobsEndpoint` to use the new `is_async` parameter. - Added regression tests for `JobsEndpoint` async get. - Enforced strict typing in mixins using `cast` and protocols. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Formatted `imednet/endpoints/jobs.py`, `imednet/core/endpoint/mixins.py`, and `tests/unit/endpoints/test_jobs_async.py` using `black` and `isort` to resolve CI failures. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Fixed line length error in `imednet/core/endpoint/mixins.py` and removed unused import in `tests/unit/endpoints/test_jobs_async.py`. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
fderuiter
deleted the
refactor/endpoint-mixins-protocols-9882042388692222746
branch
There was a problem hiding this comment.
Pull request overview
This PR refactors core endpoint mixins to improve modularity and typing, and fixes async job retrieval by removing reliance on runtime coroutine-function inspection.
Changes:
- Added
EndpointProtocolto formalize the interface expected by endpoint mixins. - Refactored
ListEndpointMixinto split request preparation from execution and tightened typing via the protocol. - Updated
PathGetEndpointMixin/JobsEndpointasync-get flow to use an explicit async path, and added a regression test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
imednet/core/endpoint/protocols.py |
Introduces EndpointProtocol to define the endpoint surface area mixins rely on. |
imednet/core/endpoint/mixins.py |
Refactors list logic and changes path-based get implementation to support explicit async execution. |
imednet/endpoints/jobs.py |
Passes is_async=True for async_get to ensure correct awaiting behavior. |
tests/unit/endpoints/test_jobs_async.py |
Adds coverage to ensure JobsEndpoint.async_get works with async clients and raises on empty responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if TYPE_CHECKING: # pragma: no cover | ||
| # EndpointProtocol is imported from .protocols, but we keep this import for | ||
| # backward compatibility if needed | ||
| pass |
There was a problem hiding this comment.
The if TYPE_CHECKING block is now a no-op (pass) and the accompanying comment about “backward compatibility” is misleading since nothing is conditionally imported/defined anymore. Consider removing this block entirely, or restoring a concrete type-only import/alias if something actually needs to be guarded for type-checking.
| # self is mixed into BaseEndpoint which implements _build_path | ||
| # Use cast for type checking compliance without importing BaseEndpoint |
There was a problem hiding this comment.
The comment says this cast is used “without importing BaseEndpoint”, but BaseEndpoint is already imported at the top of this module. Either update the comment to reflect the real intent (type narrowing) or remove it to avoid confusion.
| # self is mixed into BaseEndpoint which implements _build_path | |
| # Use cast for type checking compliance without importing BaseEndpoint | |
| # self is expected to be mixed into BaseEndpoint, which implements _build_path; | |
| # cast narrows self to BaseEndpoint so type checkers see _build_path as available |
| def _get_impl_path( | ||
| self, | ||
| client: RequestorProtocol | AsyncRequestorProtocol, | ||
| *, | ||
| study_key: Optional[str], | ||
| item_id: Any, | ||
| is_async: bool = False, | ||
| ) -> T | Awaitable[T]: | ||
| path = self._get_path_for_id(study_key, item_id) | ||
|
|
||
| # Helper to process response | ||
| def process_response(response: Any) -> T: | ||
| data = response.json() | ||
| if not data: | ||
| # Enforce strict validation for empty body | ||
| self._raise_not_found(study_key, item_id) | ||
| return self._parse_item(data) | ||
|
|
||
| if inspect.iscoroutinefunction(client.get): | ||
| if is_async: | ||
|
|
||
| async def _await() -> T: | ||
| response = await client.get(path) # type: ignore | ||
| # We assume client is AsyncRequestorProtocol because is_async=True | ||
| # But we can't be sure type-wise unless we narrow it down. | ||
| # In practice, caller ensures this. | ||
| aclient = cast(AsyncRequestorProtocol, client) | ||
| response = await aclient.get(path) | ||
| return process_response(response) | ||
|
|
||
| return _await() | ||
|
|
||
| response = client.get(path) # type: ignore | ||
| sclient = cast(RequestorProtocol, client) | ||
| response = sclient.get(path) |
There was a problem hiding this comment.
_get_impl_path now relies on the caller to pass is_async=True when an async client is provided; if a caller passes an AsyncRequestorProtocol but forgets the flag, the sync branch will call .json() on an awaitable/coroutine and fail at runtime. To make this safer, consider branching based on whether the result of client.get(path) is awaitable (or split into separate sync/async helpers) so the correct behavior doesn’t depend on an extra boolean flag.
1 participant
This PR refactors the endpoint mixins in
imednet/core/endpoint/mixins.pyto adhere to SOLID principles and improve type safety.Key changes:
EndpointProtocolinimednet/core/endpoint/protocols.pyto define the interface expected by mixins.ListEndpointMixinto separate concerns (request preparation vs execution) and use the protocol for typing.PathGetEndpointMixinto take an explicitis_asyncargument, removing fragile runtime inspection of client methods.JobsEndpointto passis_async=Truewhen calling_get_impl_pathasynchronously.tests/unit/endpoints/test_jobs_async.pyto verify the fix and prevent regression.This refactor improves modularity, testability, and code clarity.
PR created automatically by Jules for task 9882042388692222746 started by @fderuiter