-
Notifications
You must be signed in to change notification settings - Fork 0
Add Template Method pattern source generator #104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Test Results313 tests 313 ✅ 1m 27s ⏱️ Results for commit 699b33d. ♻️ This comment has been updated with latest results. |
🔍 PR Validation ResultsVersion: `` ✅ Validation Steps
📊 ArtifactsDry-run artifacts have been uploaded and will be available for 7 days. This comment was automatically generated by the PR validation workflow. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
- Coverage 82.61% 82.49% -0.13%
==========================================
Files 169 175 +6
Lines 16150 16694 +544
Branches 2273 2367 +94
==========================================
+ Hits 13343 13771 +428
- Misses 2207 2296 +89
- Partials 600 627 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements an incremental source generator for the Template Method pattern that generates Execute/ExecuteAsync methods from declarative workflow definitions with ordered steps and lifecycle hooks.
Changes:
- Adds Template Method pattern generator with support for sync/async workflows, deterministic step ordering, and lifecycle hooks (BeforeAll, AfterAll, OnError)
- Introduces attribute-based API (
[Template],[TemplateStep],[TemplateHook]) with configurable error handling policies - Includes comprehensive unit tests (16 tests), BDD-style integration tests, real-world examples, and complete documentation
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src/PatternKit.Generators/TemplateGenerator.cs | Core incremental generator implementation with 8 diagnostics, signature validation, and code generation for sync/async execution methods |
| src/PatternKit.Generators.Abstractions/Template/*.cs | Attribute definitions for Template, TemplateStep, TemplateHook, HookPoint enum, and TemplateErrorPolicy enum |
| test/PatternKit.Generators.Tests/TemplateGeneratorTests.cs | Comprehensive unit tests covering basic templates, hooks, async patterns, type targets, and diagnostics |
| test/PatternKit.Examples.Tests/TemplateMethodGeneratorDemo/*.cs | BDD-style integration tests for ImportWorkflow and OrderProcessing examples |
| src/PatternKit.Examples/TemplateMethodGeneratorDemo/*.cs | Real-world examples demonstrating data import pipeline and async order processing workflows |
| docs/generators/template-method-generator.md | Complete documentation with quick start, real-world examples, configuration options, and migration guide |
| docs/generators/toc.yml | Table of contents update to include Template Method generator documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/PatternKit.Examples/TemplateMethodGeneratorDemo/ImportWorkflowDemo.cs
Outdated
Show resolved
Hide resolved
src/PatternKit.Examples/TemplateMethodGeneratorDemo/ImportWorkflowDemo.cs
Outdated
Show resolved
Hide resolved
|
@copilot apply changes based on the comments in this thread |
…iagnostic, refactor loops, add tests Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sb.AppendLine(" try"); | ||
| sb.AppendLine(" {"); | ||
|
|
||
| // Steps | ||
| foreach (var step in sortedSteps) | ||
| { | ||
| var isAsync = IsNonGenericValueTask(step.Method.ReturnType); | ||
| if (isAsync) | ||
| { | ||
| var hasCt = step.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | ||
| var args = hasCt ? "ctx, ct" : "ctx"; | ||
| sb.AppendLine($" await {step.Method.Name}({args}).ConfigureAwait(false);"); | ||
| } | ||
| else | ||
| { | ||
| sb.AppendLine($" {step.Method.Name}(ctx);"); | ||
| } | ||
| } | ||
|
|
||
| // AfterAll hooks (inside try - only execute on success) | ||
| foreach (var hook in afterAllHooks) | ||
| { | ||
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | ||
| if (isAsync) | ||
| { | ||
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | ||
| var args = hasCt ? "ctx, ct" : "ctx"; | ||
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | ||
| } | ||
| else | ||
| { | ||
| sb.AppendLine($" {hook.Method.Name}(ctx);"); | ||
| } | ||
| } | ||
|
|
||
| sb.AppendLine(" }"); | ||
| sb.AppendLine(" catch (System.Exception ex)"); | ||
| sb.AppendLine(" {"); | ||
|
|
||
| // OnError hooks | ||
| foreach (var hook in onErrorHooks) | ||
| { | ||
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | ||
| if (isAsync) | ||
| { | ||
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | ||
| var args = hasCt ? "ctx, ex, ct" : "ctx, ex"; | ||
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | ||
| } | ||
| else | ||
| { | ||
| sb.AppendLine($" {hook.Method.Name}(ctx, ex);"); | ||
| } | ||
| } | ||
|
|
||
| if (config.ErrorPolicy == 0) // Rethrow | ||
| { | ||
| sb.AppendLine(" throw;"); | ||
| } | ||
|
|
||
| sb.AppendLine(" }"); |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states that HandleAndContinue policy "continues execution with next step" (template-method-generator.md:318), but the implementation only suppresses the exception rethrow without actually continuing to execute remaining steps. All steps are in a single try-catch block, so when an exception occurs, execution jumps to the catch block and then exits the method. Consider either: (1) wrapping each step in its own try-catch when HandleAndContinue is enabled to actually continue with remaining steps, or (2) updating the documentation to accurately reflect that HandleAndContinue only suppresses the exception without continuing execution.
| sb.AppendLine(" try"); | |
| sb.AppendLine(" {"); | |
| // Steps | |
| foreach (var step in sortedSteps) | |
| { | |
| var isAsync = IsNonGenericValueTask(step.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = step.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ct" : "ctx"; | |
| sb.AppendLine($" await {step.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {step.Method.Name}(ctx);"); | |
| } | |
| } | |
| // AfterAll hooks (inside try - only execute on success) | |
| foreach (var hook in afterAllHooks) | |
| { | |
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ct" : "ctx"; | |
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx);"); | |
| } | |
| } | |
| sb.AppendLine(" }"); | |
| sb.AppendLine(" catch (System.Exception ex)"); | |
| sb.AppendLine(" {"); | |
| // OnError hooks | |
| foreach (var hook in onErrorHooks) | |
| { | |
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ex, ct" : "ctx, ex"; | |
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx, ex);"); | |
| } | |
| } | |
| if (config.ErrorPolicy == 0) // Rethrow | |
| { | |
| sb.AppendLine(" throw;"); | |
| } | |
| sb.AppendLine(" }"); | |
| if (config.ErrorPolicy == 0) // Rethrow: single try-catch around all steps and AfterAll hooks | |
| { | |
| sb.AppendLine(" try"); | |
| sb.AppendLine(" {"); | |
| // Steps | |
| foreach (var step in sortedSteps) | |
| { | |
| var isAsync = IsNonGenericValueTask(step.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = step.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ct" : "ctx"; | |
| sb.AppendLine($" await {step.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {step.Method.Name}(ctx);"); | |
| } | |
| } | |
| // AfterAll hooks (inside try - only execute on success) | |
| foreach (var hook in afterAllHooks) | |
| { | |
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ct" : "ctx"; | |
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx);"); | |
| } | |
| } | |
| sb.AppendLine(" }"); | |
| sb.AppendLine(" catch (System.Exception ex)"); | |
| sb.AppendLine(" {"); | |
| // OnError hooks | |
| foreach (var hook in onErrorHooks) | |
| { | |
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ex, ct" : "ctx, ex"; | |
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx, ex);"); | |
| } | |
| } | |
| sb.AppendLine(" throw;"); | |
| sb.AppendLine(" }"); | |
| } | |
| else | |
| { | |
| // Handle-and-continue: wrap each step in its own try-catch so remaining steps still execute | |
| foreach (var step in sortedSteps) | |
| { | |
| var isAsync = IsNonGenericValueTask(step.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = step.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ct" : "ctx"; | |
| sb.AppendLine(" try"); | |
| sb.AppendLine(" {"); | |
| sb.AppendLine($" await {step.Method.Name}({args}).ConfigureAwait(false);"); | |
| sb.AppendLine(" }"); | |
| sb.AppendLine(" catch (System.Exception ex)"); | |
| sb.AppendLine(" {"); | |
| // OnError hooks for this step | |
| foreach (var hook in onErrorHooks) | |
| { | |
| var hookIsAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (hookIsAsync) | |
| { | |
| var hookHasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var hookArgs = hookHasCt ? "ctx, ex, ct" : "ctx, ex"; | |
| sb.AppendLine($" await {hook.Method.Name}({hookArgs}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx, ex);"); | |
| } | |
| } | |
| // Do not rethrow: continue with next step | |
| sb.AppendLine(" }"); | |
| } | |
| else | |
| { | |
| sb.AppendLine(" try"); | |
| sb.AppendLine(" {"); | |
| sb.AppendLine($" {step.Method.Name}(ctx);"); | |
| sb.AppendLine(" }"); | |
| sb.AppendLine(" catch (System.Exception ex)"); | |
| sb.AppendLine(" {"); | |
| // OnError hooks for this step | |
| foreach (var hook in onErrorHooks) | |
| { | |
| var hookIsAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (hookIsAsync) | |
| { | |
| var hookHasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var hookArgs = hookHasCt ? "ctx, ex, ct" : "ctx, ex"; | |
| sb.AppendLine($" await {hook.Method.Name}({hookArgs}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx, ex);"); | |
| } | |
| } | |
| // Do not rethrow: continue with next step | |
| sb.AppendLine(" }"); | |
| } | |
| } | |
| // AfterAll hooks (execute after all steps, even if some failed but were handled) | |
| foreach (var hook in afterAllHooks) | |
| { | |
| var isAsync = IsNonGenericValueTask(hook.Method.ReturnType); | |
| if (isAsync) | |
| { | |
| var hasCt = hook.Method.Parameters.Any(p => p.Type.Name == "CancellationToken"); | |
| var args = hasCt ? "ctx, ct" : "ctx"; | |
| sb.AppendLine($" await {hook.Method.Name}({args}).ConfigureAwait(false);"); | |
| } | |
| else | |
| { | |
| sb.AppendLine($" {hook.Method.Name}(ctx);"); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated documentation to accurately reflect current implementation. HandleAndContinue suppresses the exception (doesn't rethrow) but doesn't continue execution - the workflow terminates after the catch block. Documentation now states "Exception is suppressed (not rethrown), Workflow terminates" instead of "Execution continues with next step". Fixed in 80c6370
| private void OnError(ImportContext ctx, Exception ex) | ||
| { | ||
| ctx.Log.Add($"[{DateTime.UtcNow:HH:mm:ss}] ERROR: {ex.Message}"); | ||
| ctx.Log.Add($"[{DateTime.UtcNow:HH:mm:ss}] Import failed at validation stage"); |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message hardcodes "Import failed at validation stage" but this OnError hook can be invoked when any step fails, not just the validation step. Consider making this message more generic (e.g., "Import failed") or inspecting the exception/context to determine which step failed.
| ctx.Log.Add($"[{DateTime.UtcNow:HH:mm:ss}] Import failed at validation stage"); | |
| ctx.Log.Add($"[{DateTime.UtcNow:HH:mm:ss}] Import failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed error message from hardcoded "Import failed at validation stage" to generic "Import failed" since OnError hook can be invoked when any step fails. Fixed in 80c6370
| **HandleAndContinue:** | ||
| - OnError hook is invoked | ||
| - Execution continues with next step | ||
| - Only allowed when remaining steps are all optional | ||
|
|
||
| ```csharp | ||
| [Template(ErrorPolicy = TemplateErrorPolicy.HandleAndContinue)] | ||
| public partial class ResilientWorkflow | ||
| { | ||
| [TemplateStep(0)] | ||
| private void Step1(Context ctx) { } | ||
|
|
||
| [TemplateStep(1, Optional = true)] | ||
| private void Step2(Context ctx) { } // Must be optional | ||
|
|
||
| [TemplateStep(2, Optional = true)] | ||
| private void Step3(Context ctx) { } // Must be optional | ||
| } |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states "Only allowed when remaining steps are all optional" but the example shows Step1 (line 326) is not optional. The actual implementation in TemplateGenerator.cs (line 437) validates that ALL steps must be optional, not just "remaining" ones. Either the documentation should be updated to say "Only allowed when all steps are optional" or the validation logic needs to be changed to only check remaining steps after an error occurs. The current example would fail validation with PKTMP008.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated documentation to match implementation. Changed "remaining steps are all optional" to "all steps are optional" and updated example to mark Step1 as optional. The validation logic requires ALL steps to be optional, not just remaining ones. Fixed in 80c6370
|
@copilot apply changes based on the comments in this thread |
… update docs, refactor validation loops Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Template Method Pattern Generator - Complete Implementation
Summary
Source generator for Template Method pattern with deterministic step ordering, lifecycle hooks, and async/await support. All tests passing (21 tests).
Latest Changes - Second PR Review Round
✅ Fixed CancellationToken detection - Now uses fully qualified type name
System.Threading.CancellationTokeninstead of just type name to avoid false matchesIsCancellationTokenfor consistency✅ Updated documentation to accurately reflect HandleAndContinue behavior:
✅ Improved error message in ImportWorkflowDemo - Changed from hardcoded "Import failed at validation stage" to generic "Import failed" since OnError can be invoked from any step
✅ Refactored validation loops - Changed ValidateSignatures to use LINQ .Any() instead of foreach with continue/break
Previous Changes - First PR Review Round
✅ Removed unused imports, PKTMP006 diagnostic
✅ Fixed ValueTask validation to distinguish non-generic from generic
✅ Added 5 new tests (21 total, all passing)
✅ Refactored loops to use explicit LINQ filtering
✅ Simplified if-else and foreach in examples
Core Features
Original prompt
This section details on the original issue you should resolve
<issue_title>Generator: Create Template Method Pattern</issue_title>
<issue_description>## Summary
Add a source generator that produces a complete implementation of the Template Method pattern for consumer-defined workflows.
The generator lives in
PatternKit.Generatorsand emits self-contained, readable C# with no runtime PatternKit dependency.Primary goals:
ValueTask).Motivation / Problem
Template Method is commonly re-implemented as:
We want a declarative, boilerplate-free way to define a workflow that:
Supported Targets (must-have)
The generator must support:
partial classpartial structpartial record classpartial record structThe annotated type represents the workflow host (the place consumers call
Execute/ExecuteAsync).Proposed User Experience
Minimal template with required steps
Generated (representative shape):
Execute(ImportContext ctx)invokes steps in deterministic order.ExecuteAsync(ImportContext ctx, CancellationToken ct = default)emitted when async steps/hooks exist orForceAsync=true.Hooks + error handling
Attributes / Surface Area
Namespace:
PatternKit.Generators.TemplateCore attributes
[Template]on the workflow host[TemplateStep]on methods that form the required step sequence[TemplateHook]on methods that plug into hook pointsSuggested shapes:
TemplateAttributestring ExecuteMethodName = "Execute"string ExecuteAsyncMethodName = "ExecuteAsync"bool GenerateAsync(default: inferred)bool ForceAsync(default: false)TemplateErrorPolicy ErrorPolicy(default:Rethrow)TemplateStepAttributeint Order(required in v1)bool Optional(default: false)string? Name(optional; for diagnostics)TemplateHookAttributeHookPoint HookPoint(BeforeAll, AfterAll, OnError)int? StepOrder(reserved for v2: BeforeStep/AfterStep targeting)Step Semantics
Order.Optionalonly affects error-policy eligibility.Async rules
ValueTaskor acceptsCancellationToken, generator emits async path.Executecan exist alongside asyncExecuteAsync.ValueTask.Error rules
OnError(if present).ErrorPolicy = HandleAndContinue, only allowed when all remaining steps are optional. Otherwise emit diagnostic.Diagnostics (must-have)
Stable IDs, actionable:
PKTMP001Type marked[Template]must bepartial.PKTMP002No[TemplateStep]methods found.PKTMP003Duplicate step order detected.PKTMP004Step method signature invalid.PKTMP005Hook method signature invalid.PKTMP006Mixed sync/async signatures not supported (explain required shapes).PKTMP007CancellationToken required for async step but missing.PKTMP008HandleAndContinuenot allowed when non-optional steps remain.Generated Code Layout
TypeName.Template.g.csDeterministic ordering:
Order, then by fully-qualified method name for tie-breaking in diagnostics.Testing Expectations
OnErrorinvoked; default rethrow policy enforced.ValueTaskand respects cancellation....✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.