refactor(coroutine): 优化任务协程扩展实现#29
Conversation
- 将私有方法 CreateTaskCoroutine 替换为公共扩展方法 AsCoroutine - 简化 StartTaskAsCoroutine 方法实现,直接调用 AsCoroutine 扩展方法 - 移除重复的私有方法定义,统一使用扩展方法模式 - 提高代码可读性和复用性
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors task-to-coroutine wiring by replacing private factory methods with public Task/Task extension methods, and simplifying the scheduler entry points to use those extensions, improving reuse and readability. Sequence diagram for starting a Task as coroutine via AsCoroutine extensionsequenceDiagram
participant CS as CoroutineScheduler
participant T as Task
participant Ext as TaskCoroutineExtensions
participant WF as WaitForTask
participant CH as CoroutineHandle
CS->>T: StartTaskAsCoroutine(task)
CS->>Ext: task.AsCoroutine()
Ext->>T: AsCoroutineInstruction()
T-->>Ext: WaitForTask instance
Ext->>WF: create IEnumerator over WaitForTask
Ext-->>CS: IEnumerator IYieldInstruction
CS->>CS: Run(IEnumerator IYieldInstruction)
CS-->>CH: CoroutineHandle
Class diagram for refactored task coroutine extensionsclassDiagram
class CoroutineScheduler {
+CoroutineHandle Run(IEnumerator~IYieldInstruction~ routine)
+CoroutineHandle StartTaskAsCoroutine(Task task)
+CoroutineHandle StartTaskAsCoroutine~T~(Task~T~ task)
}
class TaskCoroutineExtensions {
<<static>> TaskCoroutineExtensions
+WaitForTask AsCoroutineInstruction(Task task)
+WaitForTask~T~ AsCoroutineInstruction~T~(Task~T~ task)
+IEnumerator~IYieldInstruction~ AsCoroutine(Task task)
+IEnumerator~IYieldInstruction~ AsCoroutine~T~(Task~T~ task)
+CoroutineHandle StartTaskAsCoroutine(CoroutineScheduler scheduler, Task task)
+CoroutineHandle StartTaskAsCoroutine~T~(CoroutineScheduler scheduler, Task~T~ task)
}
class Task {
+void Start()
}
class Task~T~ {
+T Result
+void Start()
}
class IYieldInstruction {
}
class WaitForTask {
+Task task
}
class WaitForTask~T~ {
+Task~T~ task
}
class CoroutineHandle {
}
Task <|-- Task~T~
IYieldInstruction <|-- WaitForTask
IYieldInstruction <|-- WaitForTask~T~
CoroutineScheduler ..> CoroutineHandle
CoroutineScheduler ..> Task
CoroutineScheduler ..> Task~T~
TaskCoroutineExtensions ..> Task : extends
TaskCoroutineExtensions ..> Task~T~ : extends
TaskCoroutineExtensions ..> WaitForTask
TaskCoroutineExtensions ..> WaitForTask~T~
TaskCoroutineExtensions ..> IYieldInstruction
TaskCoroutineExtensions ..> CoroutineHandle
TaskCoroutineExtensions ..> CoroutineScheduler
CoroutineScheduler --> TaskCoroutineExtensions : uses extensions
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Exposing
AsCoroutine(this Task ...)andAsCoroutine<T>(this Task<T> ...)as public extension methods changes the public API surface; consider whether these should remain internal/private helpers or be placed in a dedicated internal namespace to avoid unintended external usage. - The new
AsCoroutineextension name is very close toAsCoroutineInstruction, which may be confusing at call sites; consider a more explicit name (e.g.,ToCoroutineEnumerator) to distinguish between the yielded enumerator and the yield instruction wrapper.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Exposing `AsCoroutine(this Task ...)` and `AsCoroutine<T>(this Task<T> ...)` as public extension methods changes the public API surface; consider whether these should remain internal/private helpers or be placed in a dedicated internal namespace to avoid unintended external usage.
- The new `AsCoroutine` extension name is very close to `AsCoroutineInstruction`, which may be confusing at call sites; consider a more explicit name (e.g., `ToCoroutineEnumerator`) to distinguish between the yielded enumerator and the yield instruction wrapper.
## Individual Comments
### Comment 1
<location> `GFramework.Core/coroutine/extensions/TaskCoroutineExtensions.cs:58-60` </location>
<code_context>
/// 创建等待Task的协程
/// </summary>
- private static IEnumerator<IYieldInstruction> CreateTaskCoroutine(Task task)
+ public static IEnumerator<IYieldInstruction> AsCoroutine(this Task task)
{
yield return task.AsCoroutineInstruction();
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider adding a null check for the Task parameter to fail fast with a clearer exception.
As a public extension method, this can now be invoked on a null `Task`, leading to a `NullReferenceException` from `task.AsCoroutineInstruction()`. Adding a null guard (e.g., throw `ArgumentNullException`) would give a clearer error and match common extension-method conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public static IEnumerator<IYieldInstruction> AsCoroutine(this Task task) | ||
| { | ||
| yield return task.AsCoroutineInstruction(); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider adding a null check for the Task parameter to fail fast with a clearer exception.
As a public extension method, this can now be invoked on a null Task, leading to a NullReferenceException from task.AsCoroutineInstruction(). Adding a null guard (e.g., throw ArgumentNullException) would give a clearer error and match common extension-method conventions.
- 将AsCoroutine方法重命名为ToCoroutineEnumerator以提高语义清晰度 - 修改StartTaskAsCoroutine方法内部调用改为使用新的ToCoroutineEnumerator方法 - 为ToCoroutineEnumerator方法添加完整的XML文档注释 - 为泛型版本的ToCoroutineEnumerator<T>方法添加类型参数和参数说明 - 统一方法命名规范,提高代码可读性
Summary by Sourcery
Refactor task-to-coroutine handling to use public extension methods for creating coroutine enumerators and simplify coroutine startup APIs.
Enhancements: