refactor: StageResponse disassemble done_callback#20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the handling of callback functionality in StageResponse by splitting out the done_callback logic and introducing a new StageTask class. Key changes include:
- Importing and integrating StageTask into the module initializer.
- Adding a new StageTask module that wraps callback handling for tasks.
- Removing callback parameters and their invocations from StageResponse and updating the task dispatch logic to use StageTask.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| agently_stage/init.py | Added StageTask import to expose the new task handling functionality. |
| agently_stage/StageTask.py | Introduced StageTask and a helper Task class to manage callbacks. |
| agently_stage/StageResponse.py | Removed callback parameters and invocations to disassemble done_callback. |
| agently_stage/StageDispatch.py | Updated async function wrapper to accept StageTask instances. |
| agently_stage/Stage.py | Integrated StageTask into task creation for async, future, and sync paths. |
Comments suppressed due to low confidence (2)
agently_stage/StageResponse.py:38
- The removal of callback parameters and their invocations in StageResponse may lead to unhandled callback logic. If callbacks are still desired, update StageResponse accordingly or remove related support from StageTask and update documentation.
def __init__(self, stage: Stage, task: Future):
agently_stage/StageTask.py:27
- [nitpick] The class name 'Task' is very generic and similar to 'StageTask', which could cause confusion. Consider renaming it to a more descriptive name such as 'CallbackTask' to improve clarity.
class Task:
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the way StageResponse handles task callbacks by disassembling the done_callback logic and introducing the StageTaskProxy abstraction. Key changes include:
- Adding a new asynchronous test for error handling in Stage.
- Introducing a new file (StageTask.py) with the StageTaskProxy and its related Task wrapper.
- Refactoring StageResponse, StageDispatch, and Stage to integrate StageTaskProxy for more consistent task execution.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_api/test_Stage_with.py | Adds a new async error handling test for Stage. |
| agently_stage/init.py | Imports StageTaskProxy to expose new task handling abstractions. |
| agently_stage/StageTask.py | Implements StageTaskProxy and Task to encapsulate callback logic. |
| agently_stage/StageResponse.py | Removes direct callback invocations in favor of external delegation. |
| agently_stage/StageDispatch.py | Adjusts async function wrapper to also support StageTaskProxy. |
| agently_stage/Stage.py | Updates task dispatching to consistently use StageTaskProxy. |
Comments suppressed due to low confidence (2)
agently_stage/StageDispatch.py:219
- Using isinstance(func, StageTaskProxy) in this condition might misclassify tasks that wrap synchronous functions. Consider adding an explicit attribute or method in StageTaskProxy to clearly indicate asynchronous behavior.
if inspect.iscoroutinefunction(func) or isinstance(func, StageTaskProxy):
agently_stage/Stage.py:199
- [nitpick] The construction of StageTaskProxy differs between async and sync task handling; standardizing its parameter order and usage may improve code clarity.
StageTaskProxy(
| time.sleep(0.1) | ||
| assert counter.value == ["sync_task start", "handle_error", "long_task"] | ||
|
|
||
|
|
||
| def test_on_finally(): |
There was a problem hiding this comment.
Using time.sleep in an asynchronous test may block the event loop; consider replacing it with await asyncio.sleep(0.1) to prevent potential blockage.
| time.sleep(0.1) | |
| assert counter.value == ["sync_task start", "handle_error", "long_task"] | |
| def test_on_finally(): | |
| await asyncio.sleep(0.1) | |
| assert counter.value == ["sync_task start", "handle_error", "long_task"] | |
| async def test_on_finally(): |
refactor
StageResponse的done_callback部分