From c98e1c31cab40a45590f49e5503f9fe9ce88259a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Dec 2025 19:53:02 +0000 Subject: [PATCH 1/3] Initial plan From d4e9680858561c3643fc1f1b803e0bd7470b0108 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Dec 2025 20:04:10 +0000 Subject: [PATCH 2/3] Refactor: Eliminate duplicate code in update entity config parsers - Add parseUpdateEntityConfigTyped generic helper function - Reduces each parser from ~20 lines to ~10 lines - Maintains all existing behavior and test coverage - Simplifies adding new entity types in the future Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- pkg/workflow/update_discussion.go | 32 ++--- pkg/workflow/update_entity_helpers.go | 104 +++++++++++++++ pkg/workflow/update_entity_helpers_test.go | 144 +++++++++++++++++++++ pkg/workflow/update_issue.go | 30 ++--- pkg/workflow/update_pull_request.go | 28 ++-- pkg/workflow/update_release.go | 22 +--- 6 files changed, 280 insertions(+), 80 deletions(-) diff --git a/pkg/workflow/update_discussion.go b/pkg/workflow/update_discussion.go index 8468b1d1ee4..607231db8c8 100644 --- a/pkg/workflow/update_discussion.go +++ b/pkg/workflow/update_discussion.go @@ -17,20 +17,16 @@ type UpdateDiscussionsConfig struct { // parseUpdateDiscussionsConfig handles update-discussion configuration func (c *Compiler) parseUpdateDiscussionsConfig(outputMap map[string]any) *UpdateDiscussionsConfig { - // Create config struct - cfg := &UpdateDiscussionsConfig{} - - // Parse base config and entity-specific fields using generic helper - baseConfig, _ := c.parseUpdateEntityConfigWithFields(outputMap, UpdateEntityParseOptions{ - EntityType: UpdateEntityDiscussion, - ConfigKey: "update-discussion", - Logger: updateDiscussionLog, - Fields: []UpdateEntityFieldSpec{ - {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, - {Name: "body", Mode: FieldParsingKeyExistence, Dest: &cfg.Body}, - {Name: "labels", Mode: FieldParsingKeyExistence, Dest: &cfg.Labels}, + return parseUpdateEntityConfigTyped(c, outputMap, + UpdateEntityDiscussion, "update-discussion", updateDiscussionLog, + func(cfg *UpdateDiscussionsConfig) []UpdateEntityFieldSpec { + return []UpdateEntityFieldSpec{ + {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, + {Name: "body", Mode: FieldParsingKeyExistence, Dest: &cfg.Body}, + {Name: "labels", Mode: FieldParsingKeyExistence, Dest: &cfg.Labels}, + } }, - CustomParser: func(cm map[string]any) { + func(cm map[string]any, cfg *UpdateDiscussionsConfig) { // Parse allowed-labels using shared helper cfg.AllowedLabels = parseAllowedLabelsFromConfig(cm) if len(cfg.AllowedLabels) > 0 { @@ -40,13 +36,5 @@ func (c *Compiler) parseUpdateDiscussionsConfig(outputMap map[string]any) *Updat cfg.Labels = new(bool) } } - }, - }) - if baseConfig == nil { - return nil - } - - // Set base fields - cfg.UpdateEntityConfig = *baseConfig - return cfg + }) } diff --git a/pkg/workflow/update_entity_helpers.go b/pkg/workflow/update_entity_helpers.go index b74eedb3a9b..d2bf89c343e 100644 --- a/pkg/workflow/update_entity_helpers.go +++ b/pkg/workflow/update_entity_helpers.go @@ -294,3 +294,107 @@ func (c *Compiler) parseUpdateEntityConfigWithFields( return baseConfig, configMap } + +// UpdateEntityConfigContainer is an interface for entity-specific config types +// that embed UpdateEntityConfig +type UpdateEntityConfigContainer interface { + SetBaseConfig(base UpdateEntityConfig) +} + +// parseUpdateEntityConfigTyped is a generic helper that eliminates the final +// scaffolding duplication in update entity parsers. +// +// It handles the complete parsing flow: +// 1. Creates entity-specific config struct +// 2. Builds field specs with pointers to config fields +// 3. Calls parseUpdateEntityConfigWithFields +// 4. Checks for nil result (early return) +// 5. Copies base config into entity-specific struct +// 6. Returns typed config +// +// Type parameter: +// - T: The entity-specific config type (must embed UpdateEntityConfig) +// +// Parameters: +// - c: Compiler instance +// - outputMap: The safe-outputs configuration map +// - entityType: Type of entity (issue, pull request, discussion, release) +// - configKey: Config key in YAML (e.g., "update-issue") +// - logger: Logger for this entity type +// - buildFields: Function that receives the config struct and returns field specs +// - customParser: Optional custom parser for special fields (can be nil) +// +// Returns: +// - *T: Pointer to the parsed and populated config struct, or nil if parsing failed +// +// Usage example: +// +// func (c *Compiler) parseUpdateIssuesConfig(outputMap map[string]any) *UpdateIssuesConfig { +// return parseUpdateEntityConfigTyped(c, outputMap, +// UpdateEntityIssue, "update-issue", updateIssueLog, +// func(cfg *UpdateIssuesConfig) []UpdateEntityFieldSpec { +// return []UpdateEntityFieldSpec{ +// {Name: "status", Mode: FieldParsingKeyExistence, Dest: &cfg.Status}, +// {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, +// {Name: "body", Mode: FieldParsingKeyExistence, Dest: &cfg.Body}, +// } +// }, nil) +// } +func parseUpdateEntityConfigTyped[T any]( + c *Compiler, + outputMap map[string]any, + entityType UpdateEntityType, + configKey string, + logger *logger.Logger, + buildFields func(*T) []UpdateEntityFieldSpec, + customParser func(map[string]any, *T), +) *T { + // Create entity-specific config struct + cfg := new(T) + + // Build field specs with pointers to config fields + fields := buildFields(cfg) + + // Build parsing options + opts := UpdateEntityParseOptions{ + EntityType: entityType, + ConfigKey: configKey, + Logger: logger, + Fields: fields, + } + + // Add custom parser wrapper if provided + if customParser != nil { + opts.CustomParser = func(cm map[string]any) { + customParser(cm, cfg) + } + } + + // Parse base config and entity-specific fields + baseConfig, _ := c.parseUpdateEntityConfigWithFields(outputMap, opts) + if baseConfig == nil { + return nil + } + + // Use type assertion to set base config + // This relies on T having an embedded UpdateEntityConfig field + type baseConfigSetter interface { + setBaseConfigValue(UpdateEntityConfig) + } + + // Since we can't use interface assertion with generics directly, + // we use unsafe reflect-free assignment via any + cfgAny := any(cfg) + switch v := cfgAny.(type) { + case *UpdateIssuesConfig: + v.UpdateEntityConfig = *baseConfig + case *UpdateDiscussionsConfig: + v.UpdateEntityConfig = *baseConfig + case *UpdatePullRequestsConfig: + v.UpdateEntityConfig = *baseConfig + case *UpdateReleaseConfig: + v.UpdateEntityConfig = *baseConfig + } + + return cfg +} diff --git a/pkg/workflow/update_entity_helpers_test.go b/pkg/workflow/update_entity_helpers_test.go index 2644641a6ff..c593bbcd210 100644 --- a/pkg/workflow/update_entity_helpers_test.go +++ b/pkg/workflow/update_entity_helpers_test.go @@ -255,3 +255,147 @@ func TestParseUpdateEntityConfigWithFields(t *testing.T) { }) } } + +// TestParseUpdateEntityConfigTyped tests the generic wrapper function +func TestParseUpdateEntityConfigTyped(t *testing.T) { + tests := []struct { + name string + outputMap map[string]any + entityType UpdateEntityType + configKey string + wantNil bool + validateFunc func(*testing.T, *UpdateIssuesConfig) // Using UpdateIssuesConfig for simplicity + }{ + { + name: "basic config with fields", + outputMap: map[string]any{ + "update-issue": map[string]any{ + "max": 2, + "title": nil, + "body": nil, + "status": nil, + }, + }, + entityType: UpdateEntityIssue, + configKey: "update-issue", + wantNil: false, + validateFunc: func(t *testing.T, cfg *UpdateIssuesConfig) { + if cfg.Max != 2 { + t.Errorf("Expected max=2, got %d", cfg.Max) + } + if cfg.Title == nil { + t.Error("Expected title to be non-nil") + } + if cfg.Body == nil { + t.Error("Expected body to be non-nil") + } + if cfg.Status == nil { + t.Error("Expected status to be non-nil") + } + }, + }, + { + name: "config with target", + outputMap: map[string]any{ + "update-issue": map[string]any{ + "target": "123", + "title": nil, + }, + }, + entityType: UpdateEntityIssue, + configKey: "update-issue", + wantNil: false, + validateFunc: func(t *testing.T, cfg *UpdateIssuesConfig) { + if cfg.Target != "123" { + t.Errorf("Expected target='123', got '%s'", cfg.Target) + } + if cfg.Title == nil { + t.Error("Expected title to be non-nil") + } + }, + }, + { + name: "missing config key returns nil", + outputMap: map[string]any{ + "other-key": map[string]any{}, + }, + entityType: UpdateEntityIssue, + configKey: "update-issue", + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler(false, "", "test") + result := parseUpdateEntityConfigTyped(compiler, tt.outputMap, + tt.entityType, tt.configKey, logger.New("test"), + func(cfg *UpdateIssuesConfig) []UpdateEntityFieldSpec { + return []UpdateEntityFieldSpec{ + {Name: "status", Mode: FieldParsingKeyExistence, Dest: &cfg.Status}, + {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, + {Name: "body", Mode: FieldParsingKeyExistence, Dest: &cfg.Body}, + } + }, nil) + + if tt.wantNil { + if result != nil { + t.Errorf("Expected nil result, got %v", result) + } + } else { + if result == nil { + t.Errorf("Expected non-nil result, got nil") + } else if tt.validateFunc != nil { + tt.validateFunc(t, result) + } + } + }) + } +} + +// TestParseUpdateEntityConfigTypedWithCustomParser tests custom parser support +func TestParseUpdateEntityConfigTypedWithCustomParser(t *testing.T) { + outputMap := map[string]any{ + "update-discussion": map[string]any{ + "title": nil, + "labels": nil, + "allowed-labels": []any{"bug", "enhancement"}, + }, + } + + compiler := NewCompiler(false, "", "test") + result := parseUpdateEntityConfigTyped(compiler, outputMap, + UpdateEntityDiscussion, "update-discussion", logger.New("test"), + func(cfg *UpdateDiscussionsConfig) []UpdateEntityFieldSpec { + return []UpdateEntityFieldSpec{ + {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, + {Name: "labels", Mode: FieldParsingKeyExistence, Dest: &cfg.Labels}, + } + }, + func(cm map[string]any, cfg *UpdateDiscussionsConfig) { + cfg.AllowedLabels = parseAllowedLabelsFromConfig(cm) + }) + + if result == nil { + t.Fatal("Expected non-nil result") + } + + if result.Title == nil { + t.Error("Expected title to be non-nil") + } + + if result.Labels == nil { + t.Error("Expected labels to be non-nil") + } + + expectedLabels := []string{"bug", "enhancement"} + if len(result.AllowedLabels) != len(expectedLabels) { + t.Fatalf("Expected %d allowed labels, got %d", len(expectedLabels), len(result.AllowedLabels)) + } + + for i, expected := range expectedLabels { + if result.AllowedLabels[i] != expected { + t.Errorf("Expected allowed label[%d]='%s', got '%s'", i, expected, result.AllowedLabels[i]) + } + } +} diff --git a/pkg/workflow/update_issue.go b/pkg/workflow/update_issue.go index 8da3601f521..5bf614aa3a0 100644 --- a/pkg/workflow/update_issue.go +++ b/pkg/workflow/update_issue.go @@ -16,25 +16,13 @@ type UpdateIssuesConfig struct { // parseUpdateIssuesConfig handles update-issue configuration func (c *Compiler) parseUpdateIssuesConfig(outputMap map[string]any) *UpdateIssuesConfig { - // Create config struct - cfg := &UpdateIssuesConfig{} - - // Parse base config and entity-specific fields using generic helper - baseConfig, _ := c.parseUpdateEntityConfigWithFields(outputMap, UpdateEntityParseOptions{ - EntityType: UpdateEntityIssue, - ConfigKey: "update-issue", - Logger: updateIssueLog, - Fields: []UpdateEntityFieldSpec{ - {Name: "status", Mode: FieldParsingKeyExistence, Dest: &cfg.Status}, - {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, - {Name: "body", Mode: FieldParsingKeyExistence, Dest: &cfg.Body}, - }, - }) - if baseConfig == nil { - return nil - } - - // Set base fields - cfg.UpdateEntityConfig = *baseConfig - return cfg + return parseUpdateEntityConfigTyped(c, outputMap, + UpdateEntityIssue, "update-issue", updateIssueLog, + func(cfg *UpdateIssuesConfig) []UpdateEntityFieldSpec { + return []UpdateEntityFieldSpec{ + {Name: "status", Mode: FieldParsingKeyExistence, Dest: &cfg.Status}, + {Name: "title", Mode: FieldParsingKeyExistence, Dest: &cfg.Title}, + {Name: "body", Mode: FieldParsingKeyExistence, Dest: &cfg.Body}, + } + }, nil) } diff --git a/pkg/workflow/update_pull_request.go b/pkg/workflow/update_pull_request.go index 058cc4acf78..151d17e1ced 100644 --- a/pkg/workflow/update_pull_request.go +++ b/pkg/workflow/update_pull_request.go @@ -17,24 +17,12 @@ type UpdatePullRequestsConfig struct { func (c *Compiler) parseUpdatePullRequestsConfig(outputMap map[string]any) *UpdatePullRequestsConfig { updatePullRequestLog.Print("Parsing update pull request configuration") - // Create config struct - cfg := &UpdatePullRequestsConfig{} - - // Parse base config and entity-specific fields using generic helper - baseConfig, _ := c.parseUpdateEntityConfigWithFields(outputMap, UpdateEntityParseOptions{ - EntityType: UpdateEntityPullRequest, - ConfigKey: "update-pull-request", - Logger: updatePullRequestLog, - Fields: []UpdateEntityFieldSpec{ - {Name: "title", Mode: FieldParsingBoolValue, Dest: &cfg.Title}, - {Name: "body", Mode: FieldParsingBoolValue, Dest: &cfg.Body}, - }, - }) - if baseConfig == nil { - return nil - } - - // Set base fields - cfg.UpdateEntityConfig = *baseConfig - return cfg + return parseUpdateEntityConfigTyped(c, outputMap, + UpdateEntityPullRequest, "update-pull-request", updatePullRequestLog, + func(cfg *UpdatePullRequestsConfig) []UpdateEntityFieldSpec { + return []UpdateEntityFieldSpec{ + {Name: "title", Mode: FieldParsingBoolValue, Dest: &cfg.Title}, + {Name: "body", Mode: FieldParsingBoolValue, Dest: &cfg.Body}, + } + }, nil) } diff --git a/pkg/workflow/update_release.go b/pkg/workflow/update_release.go index ac02b610094..b70591df77f 100644 --- a/pkg/workflow/update_release.go +++ b/pkg/workflow/update_release.go @@ -13,21 +13,9 @@ type UpdateReleaseConfig struct { // parseUpdateReleaseConfig handles update-release configuration func (c *Compiler) parseUpdateReleaseConfig(outputMap map[string]any) *UpdateReleaseConfig { - // Create config struct - cfg := &UpdateReleaseConfig{} - - // Parse base config (no entity-specific fields for releases) - baseConfig, _ := c.parseUpdateEntityConfigWithFields(outputMap, UpdateEntityParseOptions{ - EntityType: UpdateEntityRelease, - ConfigKey: "update-release", - Logger: updateReleaseLog, - Fields: nil, // No entity-specific fields - }) - if baseConfig == nil { - return nil - } - - // Set base fields - cfg.UpdateEntityConfig = *baseConfig - return cfg + return parseUpdateEntityConfigTyped(c, outputMap, + UpdateEntityRelease, "update-release", updateReleaseLog, + func(cfg *UpdateReleaseConfig) []UpdateEntityFieldSpec { + return nil // No entity-specific fields for releases + }, nil) } From 2e4ecbbf0cff97b8b12d4b1d74a6501a23ecc3ac Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Dec 2025 20:17:53 +0000 Subject: [PATCH 3/3] Fix: Remove unused interface and improve code comments - Remove unused UpdateEntityConfigContainer interface - Remove unused baseConfigSetter interface - Improve code comments for type assertion logic - All tests pass, linting successful Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com> --- .devcontainer/devcontainer.json | 9 ++++++++- docs/src/content/docs/labs.mdx | 1 + pkg/workflow/update_entity_helpers.go | 13 +------------ 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index da5e377b59c..c8ff4249f47 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -3,7 +3,14 @@ "image": "mcr.microsoft.com/devcontainers/go:1-bookworm", "customizations": { "vscode": { - "extensions": ["golang.go", "GitHub.copilot-chat", "GitHub.copilot", "github.vscode-github-actions", "astro-build.astro-vscode", "DavidAnson.vscode-markdownlint"] + "extensions": [ + "golang.go", + "GitHub.copilot-chat", + "GitHub.copilot", + "github.vscode-github-actions", + "astro-build.astro-vscode", + "DavidAnson.vscode-markdownlint" + ] }, "codespaces": { "repositories": { diff --git a/docs/src/content/docs/labs.mdx b/docs/src/content/docs/labs.mdx index 196f50e8034..ff4c7914368 100644 --- a/docs/src/content/docs/labs.mdx +++ b/docs/src/content/docs/labs.mdx @@ -124,6 +124,7 @@ These are experimental agentic workflows used by the GitHub Next team to learn, | [Sub-Issue Closer](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/sub-issue-closer.md) | copilot | [![Sub-Issue Closer](https://github.com/githubnext/gh-aw/actions/workflows/sub-issue-closer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/sub-issue-closer.lock.yml) | - | - | | [Super Linter Report](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/super-linter.md) | copilot | [![Super Linter Report](https://github.com/githubnext/gh-aw/actions/workflows/super-linter.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/super-linter.lock.yml) | `0 14 * * 1-5` | - | | [Technical Doc Writer](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/technical-doc-writer.md) | copilot | [![Technical Doc Writer](https://github.com/githubnext/gh-aw/actions/workflows/technical-doc-writer.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/technical-doc-writer.lock.yml) | - | - | +| [Terminal Stylist](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/terminal-stylist.md) | copilot | [![Terminal Stylist](https://github.com/githubnext/gh-aw/actions/workflows/terminal-stylist.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/terminal-stylist.lock.yml) | - | - | | [The Daily Repository Chronicle](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/daily-repo-chronicle.md) | copilot | [![The Daily Repository Chronicle](https://github.com/githubnext/gh-aw/actions/workflows/daily-repo-chronicle.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/daily-repo-chronicle.lock.yml) | `0 16 * * 1-5` | - | | [The Great Escapi](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/firewall-escape.md) | copilot | [![The Great Escapi](https://github.com/githubnext/gh-aw/actions/workflows/firewall-escape.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/firewall-escape.lock.yml) | - | - | | [Tidy](https://github.com/githubnext/gh-aw/blob/main/.github/workflows/tidy.md) | copilot | [![Tidy](https://github.com/githubnext/gh-aw/actions/workflows/tidy.lock.yml/badge.svg)](https://github.com/githubnext/gh-aw/actions/workflows/tidy.lock.yml) | `0 7 * * *` | - | diff --git a/pkg/workflow/update_entity_helpers.go b/pkg/workflow/update_entity_helpers.go index d2bf89c343e..1b1e309183c 100644 --- a/pkg/workflow/update_entity_helpers.go +++ b/pkg/workflow/update_entity_helpers.go @@ -295,12 +295,6 @@ func (c *Compiler) parseUpdateEntityConfigWithFields( return baseConfig, configMap } -// UpdateEntityConfigContainer is an interface for entity-specific config types -// that embed UpdateEntityConfig -type UpdateEntityConfigContainer interface { - SetBaseConfig(base UpdateEntityConfig) -} - // parseUpdateEntityConfigTyped is a generic helper that eliminates the final // scaffolding duplication in update entity parsers. // @@ -377,13 +371,8 @@ func parseUpdateEntityConfigTyped[T any]( } // Use type assertion to set base config - // This relies on T having an embedded UpdateEntityConfig field - type baseConfigSetter interface { - setBaseConfigValue(UpdateEntityConfig) - } - // Since we can't use interface assertion with generics directly, - // we use unsafe reflect-free assignment via any + // we use type switch via any to assign the base config cfgAny := any(cfg) switch v := cfgAny.(type) { case *UpdateIssuesConfig: