Skip to content

Commit 90141fe

Browse files
CopilotCopilot
authored
Refactor: Extract generic field parser for update entity configs (#7052)
1 parent b42bd9e commit 90141fe

5 files changed

Lines changed: 206 additions & 54 deletions

File tree

‎pkg/workflow/update_discussion.go‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,24 +28,13 @@ func (c *Compiler) parseUpdateDiscussionsConfig(outputMap map[string]any) *Updat
2828
UpdateEntityConfig: *baseConfig,
2929
}
3030

31-
// Parse discussion-specific fields from config map
32-
if configMap != nil {
33-
// Parse title - presence of the key (even if nil/empty) indicates field can be updated
34-
if _, exists := configMap["title"]; exists {
35-
updateDiscussionsConfig.Title = new(bool)
36-
}
37-
38-
// Parse body - presence of the key (even if nil/empty) indicates field can be updated
39-
if _, exists := configMap["body"]; exists {
40-
updateDiscussionsConfig.Body = new(bool)
41-
}
31+
// Parse discussion-specific fields using key existence mode
32+
updateDiscussionsConfig.Title = parseUpdateEntityBoolField(configMap, "title", FieldParsingKeyExistence)
33+
updateDiscussionsConfig.Body = parseUpdateEntityBoolField(configMap, "body", FieldParsingKeyExistence)
34+
updateDiscussionsConfig.Labels = parseUpdateEntityBoolField(configMap, "labels", FieldParsingKeyExistence)
4235

43-
// Parse labels - presence of the key (even if nil/empty) indicates field can be updated
44-
if _, exists := configMap["labels"]; exists {
45-
updateDiscussionsConfig.Labels = new(bool)
46-
}
47-
48-
// Parse allowed-labels using shared helper
36+
// Parse allowed-labels using shared helper
37+
if configMap != nil {
4938
updateDiscussionsConfig.AllowedLabels = parseAllowedLabelsFromConfig(configMap)
5039
if len(updateDiscussionsConfig.AllowedLabels) > 0 {
5140
updateDiscussionLog.Printf("Allowed labels configured: %v", updateDiscussionsConfig.AllowedLabels)

‎pkg/workflow/update_entity_helpers.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,3 +120,57 @@ func (c *Compiler) parseUpdateEntityBase(
120120

121121
return baseConfig, configMap
122122
}
123+
124+
// FieldParsingMode determines how boolean fields are parsed from the config
125+
type FieldParsingMode int
126+
127+
const (
128+
// FieldParsingKeyExistence mode: Field presence (even if nil) indicates it can be updated
129+
// Used by update-issue and update-discussion
130+
FieldParsingKeyExistence FieldParsingMode = iota
131+
// FieldParsingBoolValue mode: Field's boolean value determines if it can be updated
132+
// Used by update-pull-request (defaults to true if nil)
133+
FieldParsingBoolValue
134+
)
135+
136+
// parseUpdateEntityBoolField is a generic helper that parses boolean fields from config maps
137+
// based on the specified parsing mode.
138+
//
139+
// Parameters:
140+
// - configMap: The entity-specific configuration map
141+
// - fieldName: The name of the field to parse (e.g., "title", "body", "status")
142+
// - mode: The parsing mode (FieldParsingKeyExistence or FieldParsingBoolValue)
143+
//
144+
// Returns:
145+
// - *bool: A pointer to bool if the field should be enabled, nil if disabled
146+
//
147+
// Behavior by mode:
148+
// - FieldParsingKeyExistence: Returns new(bool) if key exists, nil otherwise
149+
// - FieldParsingBoolValue: Returns &boolValue if key exists and is bool, nil otherwise
150+
func parseUpdateEntityBoolField(configMap map[string]any, fieldName string, mode FieldParsingMode) *bool {
151+
if configMap == nil {
152+
return nil
153+
}
154+
155+
val, exists := configMap[fieldName]
156+
if !exists {
157+
return nil
158+
}
159+
160+
switch mode {
161+
case FieldParsingKeyExistence:
162+
// Key presence (even if nil) indicates field can be updated
163+
return new(bool) // Allocate a new bool pointer (defaults to false)
164+
165+
case FieldParsingBoolValue:
166+
// Parse actual boolean value from config
167+
if boolVal, ok := val.(bool); ok {
168+
return &boolVal
169+
}
170+
// If present but not a bool (e.g., null), return nil (no explicit setting)
171+
return nil
172+
173+
default:
174+
return nil
175+
}
176+
}

‎pkg/workflow/update_entity_helpers_test.go‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
package workflow
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestParseUpdateEntityBoolField(t *testing.T) {
8+
tests := []struct {
9+
name string
10+
configMap map[string]any
11+
fieldName string
12+
mode FieldParsingMode
13+
wantNil bool
14+
wantValue bool // Only checked if wantNil is false
15+
}{
16+
// FieldParsingKeyExistence mode tests
17+
{
18+
name: "key existence mode: key present with nil value",
19+
configMap: map[string]any{"title": nil},
20+
fieldName: "title",
21+
mode: FieldParsingKeyExistence,
22+
wantNil: false, // Should return non-nil pointer
23+
wantValue: false, // Default bool value
24+
},
25+
{
26+
name: "key existence mode: key present with empty value",
27+
configMap: map[string]any{"title": ""},
28+
fieldName: "title",
29+
mode: FieldParsingKeyExistence,
30+
wantNil: false,
31+
wantValue: false,
32+
},
33+
{
34+
name: "key existence mode: key not present",
35+
configMap: map[string]any{"other": true},
36+
fieldName: "title",
37+
mode: FieldParsingKeyExistence,
38+
wantNil: true,
39+
},
40+
{
41+
name: "key existence mode: nil config map",
42+
configMap: nil,
43+
fieldName: "title",
44+
mode: FieldParsingKeyExistence,
45+
wantNil: true,
46+
},
47+
{
48+
name: "key existence mode: empty config map",
49+
configMap: map[string]any{},
50+
fieldName: "title",
51+
mode: FieldParsingKeyExistence,
52+
wantNil: true,
53+
},
54+
55+
// FieldParsingBoolValue mode tests
56+
{
57+
name: "bool value mode: true value",
58+
configMap: map[string]any{"title": true},
59+
fieldName: "title",
60+
mode: FieldParsingBoolValue,
61+
wantNil: false,
62+
wantValue: true,
63+
},
64+
{
65+
name: "bool value mode: false value",
66+
configMap: map[string]any{"title": false},
67+
fieldName: "title",
68+
mode: FieldParsingBoolValue,
69+
wantNil: false,
70+
wantValue: false,
71+
},
72+
{
73+
name: "bool value mode: nil value (not a bool)",
74+
configMap: map[string]any{"title": nil},
75+
fieldName: "title",
76+
mode: FieldParsingBoolValue,
77+
wantNil: true, // Non-bool values return nil
78+
},
79+
{
80+
name: "bool value mode: string value (not a bool)",
81+
configMap: map[string]any{"title": "true"},
82+
fieldName: "title",
83+
mode: FieldParsingBoolValue,
84+
wantNil: true,
85+
},
86+
{
87+
name: "bool value mode: key not present",
88+
configMap: map[string]any{"other": true},
89+
fieldName: "title",
90+
mode: FieldParsingBoolValue,
91+
wantNil: true,
92+
},
93+
{
94+
name: "bool value mode: nil config map",
95+
configMap: nil,
96+
fieldName: "title",
97+
mode: FieldParsingBoolValue,
98+
wantNil: true,
99+
},
100+
}
101+
102+
for _, tt := range tests {
103+
t.Run(tt.name, func(t *testing.T) {
104+
result := parseUpdateEntityBoolField(tt.configMap, tt.fieldName, tt.mode)
105+
106+
if tt.wantNil {
107+
if result != nil {
108+
t.Errorf("Expected nil result, got %v", result)
109+
}
110+
} else {
111+
if result == nil {
112+
t.Errorf("Expected non-nil result, got nil")
113+
} else if *result != tt.wantValue {
114+
t.Errorf("Expected value %v, got %v", tt.wantValue, *result)
115+
}
116+
}
117+
})
118+
}
119+
}
120+
121+
func TestParseUpdateEntityBoolFieldFieldNames(t *testing.T) {
122+
// Test that different field names work correctly
123+
configMap := map[string]any{
124+
"title": nil,
125+
"body": nil,
126+
"status": nil,
127+
"labels": nil,
128+
}
129+
130+
fieldNames := []string{"title", "body", "status", "labels"}
131+
for _, fieldName := range fieldNames {
132+
t.Run(fieldName, func(t *testing.T) {
133+
result := parseUpdateEntityBoolField(configMap, fieldName, FieldParsingKeyExistence)
134+
if result == nil {
135+
t.Errorf("Expected non-nil result for field %s", fieldName)
136+
}
137+
})
138+
}
139+
}

‎pkg/workflow/update_issue.go‎

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,25 +27,10 @@ func (c *Compiler) parseUpdateIssuesConfig(outputMap map[string]any) *UpdateIssu
2727
UpdateEntityConfig: *baseConfig,
2828
}
2929

30-
// Parse issue-specific fields from config map
31-
if configMap != nil {
32-
// Parse status - presence of the key (even if nil/empty) indicates field can be updated
33-
if _, exists := configMap["status"]; exists {
34-
// If the key exists, it means we can update the status
35-
// We don't care about the value - just that the key is present
36-
updateIssuesConfig.Status = new(bool) // Allocate a new bool pointer (defaults to false)
37-
}
38-
39-
// Parse title - presence of the key (even if nil/empty) indicates field can be updated
40-
if _, exists := configMap["title"]; exists {
41-
updateIssuesConfig.Title = new(bool)
42-
}
43-
44-
// Parse body - presence of the key (even if nil/empty) indicates field can be updated
45-
if _, exists := configMap["body"]; exists {
46-
updateIssuesConfig.Body = new(bool)
47-
}
48-
}
30+
// Parse issue-specific fields using key existence mode
31+
updateIssuesConfig.Status = parseUpdateEntityBoolField(configMap, "status", FieldParsingKeyExistence)
32+
updateIssuesConfig.Title = parseUpdateEntityBoolField(configMap, "title", FieldParsingKeyExistence)
33+
updateIssuesConfig.Body = parseUpdateEntityBoolField(configMap, "body", FieldParsingKeyExistence)
4934

5035
return updateIssuesConfig
5136
}

‎pkg/workflow/update_pull_request.go‎

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -28,24 +28,9 @@ func (c *Compiler) parseUpdatePullRequestsConfig(outputMap map[string]any) *Upda
2828
UpdateEntityConfig: *baseConfig,
2929
}
3030

31-
// Parse PR-specific fields from config map
32-
if configMap != nil {
33-
// Parse title - boolean to enable/disable (defaults to true if nil or not set)
34-
if titleVal, exists := configMap["title"]; exists {
35-
if titleBool, ok := titleVal.(bool); ok {
36-
updatePullRequestsConfig.Title = &titleBool
37-
}
38-
// If present but not a bool (e.g., null), leave as nil (defaults to enabled)
39-
}
40-
41-
// Parse body - boolean to enable/disable (defaults to true if nil or not set)
42-
if bodyVal, exists := configMap["body"]; exists {
43-
if bodyBool, ok := bodyVal.(bool); ok {
44-
updatePullRequestsConfig.Body = &bodyBool
45-
}
46-
// If present but not a bool (e.g., null), leave as nil (defaults to enabled)
47-
}
48-
}
31+
// Parse PR-specific fields using bool value mode (defaults to true if nil)
32+
updatePullRequestsConfig.Title = parseUpdateEntityBoolField(configMap, "title", FieldParsingBoolValue)
33+
updatePullRequestsConfig.Body = parseUpdateEntityBoolField(configMap, "body", FieldParsingBoolValue)
4934

5035
return updatePullRequestsConfig
5136
}

0 commit comments

Comments
 (0)