🛠️ Refactor: Core - Extract PathGetOperation#722
Conversation
- ♻️ DRY: Moved the repeated HTTP fetch and parsing logic out of `PathGetEndpointMixin` into a dedicated `PathGetOperation` class. - 🧱 SOLID: Improved single responsibility by separating the definition of the path GET mechanism in the mixin from the execution of the request and response parsing. - 📉 Type-Safe: Ensured the new abstraction preserves `Generic[T]` typing and handles context isolation correctly through constructor injection of `parse_func` and `not_found_func`. 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. |
fderuiter
deleted the
refactor-extract-pathgetoperation-7851411157722976817
branch
There was a problem hiding this comment.
Pull request overview
This PR refactors the core “GET by direct path” endpoint behavior by extracting the HTTP GET + JSON parsing/not-found handling from PathGetEndpointMixin into a dedicated PathGetOperation, aligning it with the existing “operation” pattern used for list/create flows.
Changes:
- Added
PathGetOperation(src/imednet/core/endpoint/operations/get.py) to encapsulate sync/async GET execution and response parsing. - Refactored
PathGetEndpointMixin._get_path_sync/_get_path_asyncto delegate toPathGetOperation. - Exported
PathGetOperationfromsrc/imednet/core/endpoint/operations/__init__.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/imednet/core/endpoint/operations/get.py | Introduces the new operation class for path-based GET + parse/not-found behavior. |
| src/imednet/core/endpoint/operations/init.py | Exposes the new operation from the operations package public API. |
| src/imednet/core/endpoint/mixins/get.py | Switches path-based get mixin to use the new operation class. |
Comments suppressed due to low confidence (1)
src/imednet/core/endpoint/mixins/get.py:7
- Importing
PathGetOperationdirectly fromimednet.core.endpoint.operations.getis inconsistent with the established pattern elsewhere (e.g.,mixins/list.pyimports operations via the packageimednet.core.endpoint.operations). SincePathGetOperationis now exported fromoperations/__init__.py, import it fromimednet.core.endpoint.operationsto keep public import paths consistent and reduce churn if module layout changes again.
from imednet.core.endpoint.abc import EndpointABC
from imednet.core.endpoint.operations.get import PathGetOperation
from imednet.core.paginator import AsyncPaginator, Paginator
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parse_func: Callable[[Any], T], | ||
| not_found_func: Callable[[], None], | ||
| ) -> None: | ||
| """ | ||
| Initialize the path get operation. | ||
|
|
||
| Args: | ||
| path: The API endpoint path. | ||
| parse_func: A function to parse a raw JSON item into the model T. | ||
| not_found_func: A callback to raise the appropriate not found error. | ||
| """ | ||
| self.path = path | ||
| self.parse_func = parse_func | ||
| self.not_found_func = not_found_func | ||
|
|
||
| def _process_response(self, response: Any) -> T: | ||
| """Process the raw HTTP response.""" | ||
| data = response.json() | ||
| if not data: | ||
| self.not_found_func() | ||
| return self.parse_func(data) |
There was a problem hiding this comment.
not_found_func is typed as Callable[[], None] and _process_response() continues to call parse_func(data) after invoking it. This makes the operation’s contract ambiguous/unsafe if a caller provides a callback that returns normally (it would then attempt to parse an empty payload). Consider typing not_found_func as Callable[[], NoReturn] (and/or explicitly stopping execution after calling it) so both type-checkers and runtime behavior enforce the intended “must raise” contract.
This PR introduces a structural improvement to the SDK core by abstracting the HTTP GET request and JSON parsing logic out of
PathGetEndpointMixininto a specializedPathGetOperationclass (located insrc/imednet/core/endpoint/operations/get.py).Changes include:
PathGetOperationto handle synchronous and asynchronous HTTP requests, 404/not found states, and response parsing._get_path_syncand_get_path_asyncinPathGetEndpointMixinto utilize the new operation class.src/imednet/core/endpoint/operations/__init__.py.This change follows the pattern established by
ListOperationandRecordCreateOperation, strictly adhering to SOLID principles by decoupling the endpoint mixin from the underlying execution logic without altering runtime behavior. All tests passed correctly.PR created automatically by Jules for task 7851411157722976817 started by @fderuiter