Skip to content

Commit d206a7c

Browse files
monotykamarymonotykamary
committed
refactor: address feedback on optionalParamOK helper
1 parent 65a4558 commit d206a7c

File tree

3 files changed

+131
-15
lines changed

3 files changed

+131
-15
lines changed

‎pkg/github/helper_test.go‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,115 @@ func getTextResult(t *testing.T, result *mcp.CallToolResult) mcp.TextContent {
9393
assert.Equal(t, "text", textContent.Type)
9494
return textContent
9595
}
96+
97+
func TestOptionalParamOK(t *testing.T) {
98+
tests := []struct {
99+
name string
100+
args map[string]interface{}
101+
paramName string
102+
expectedVal interface{}
103+
expectedOk bool
104+
expectError bool
105+
errorMsg string
106+
}{
107+
{
108+
name: "present and correct type (string)",
109+
args: map[string]interface{}{"myParam": "hello"},
110+
paramName: "myParam",
111+
expectedVal: "hello",
112+
expectedOk: true,
113+
expectError: false,
114+
},
115+
{
116+
name: "present and correct type (bool)",
117+
args: map[string]interface{}{"myParam": true},
118+
paramName: "myParam",
119+
expectedVal: true,
120+
expectedOk: true,
121+
expectError: false,
122+
},
123+
{
124+
name: "present and correct type (number)",
125+
args: map[string]interface{}{"myParam": float64(123)},
126+
paramName: "myParam",
127+
expectedVal: float64(123),
128+
expectedOk: true,
129+
expectError: false,
130+
},
131+
{
132+
name: "present but wrong type (string expected, got bool)",
133+
args: map[string]interface{}{"myParam": true},
134+
paramName: "myParam",
135+
expectedVal: "", // Zero value for string
136+
expectedOk: true, // ok is true because param exists
137+
expectError: true,
138+
errorMsg: "parameter myParam is not of type string, is bool",
139+
},
140+
{
141+
name: "present but wrong type (bool expected, got string)",
142+
args: map[string]interface{}{"myParam": "true"},
143+
paramName: "myParam",
144+
expectedVal: false, // Zero value for bool
145+
expectedOk: true, // ok is true because param exists
146+
expectError: true,
147+
errorMsg: "parameter myParam is not of type bool, is string",
148+
},
149+
{
150+
name: "parameter not present",
151+
args: map[string]interface{}{"anotherParam": "value"},
152+
paramName: "myParam",
153+
expectedVal: "", // Zero value for string
154+
expectedOk: false,
155+
expectError: false,
156+
},
157+
}
158+
159+
for _, tc := range tests {
160+
t.Run(tc.name, func(t *testing.T) {
161+
request := createMCPRequest(tc.args)
162+
163+
// Test with string type assertion
164+
if _, isString := tc.expectedVal.(string); isString || tc.errorMsg == "parameter myParam is not of type string, is bool" {
165+
val, ok, err := optionalParamOK[string](request, tc.paramName)
166+
if tc.expectError {
167+
require.Error(t, err)
168+
assert.Contains(t, err.Error(), tc.errorMsg)
169+
assert.Equal(t, tc.expectedOk, ok) // Check ok even on error
170+
assert.Equal(t, tc.expectedVal, val) // Check zero value on error
171+
} else {
172+
require.NoError(t, err)
173+
assert.Equal(t, tc.expectedOk, ok)
174+
assert.Equal(t, tc.expectedVal, val)
175+
}
176+
}
177+
178+
// Test with bool type assertion
179+
if _, isBool := tc.expectedVal.(bool); isBool || tc.errorMsg == "parameter myParam is not of type bool, is string" {
180+
val, ok, err := optionalParamOK[bool](request, tc.paramName)
181+
if tc.expectError {
182+
require.Error(t, err)
183+
assert.Contains(t, err.Error(), tc.errorMsg)
184+
assert.Equal(t, tc.expectedOk, ok) // Check ok even on error
185+
assert.Equal(t, tc.expectedVal, val) // Check zero value on error
186+
} else {
187+
require.NoError(t, err)
188+
assert.Equal(t, tc.expectedOk, ok)
189+
assert.Equal(t, tc.expectedVal, val)
190+
}
191+
}
192+
193+
// Test with float64 type assertion (for number case)
194+
if _, isFloat := tc.expectedVal.(float64); isFloat {
195+
val, ok, err := optionalParamOK[float64](request, tc.paramName)
196+
if tc.expectError {
197+
// This case shouldn't happen for float64 in the defined tests
198+
require.Fail(t, "Unexpected error case for float64")
199+
} else {
200+
require.NoError(t, err)
201+
assert.Equal(t, tc.expectedOk, ok)
202+
assert.Equal(t, tc.expectedVal, val)
203+
}
204+
}
205+
})
206+
}
207+
}

‎pkg/github/pullrequests.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,35 +118,35 @@ func updatePullRequest(client *github.Client, t translations.TranslationHelperFu
118118
update := &github.PullRequest{}
119119
updateNeeded := false
120120

121-
if title, ok, err := optionalParamOk[string](request, "title"); err != nil {
121+
if title, ok, err := optionalParamOK[string](request, "title"); err != nil {
122122
return mcp.NewToolResultError(err.Error()), nil
123123
} else if ok {
124124
update.Title = github.Ptr(title)
125125
updateNeeded = true
126126
}
127127

128-
if body, ok, err := optionalParamOk[string](request, "body"); err != nil {
128+
if body, ok, err := optionalParamOK[string](request, "body"); err != nil {
129129
return mcp.NewToolResultError(err.Error()), nil
130130
} else if ok {
131131
update.Body = github.Ptr(body)
132132
updateNeeded = true
133133
}
134134

135-
if state, ok, err := optionalParamOk[string](request, "state"); err != nil {
135+
if state, ok, err := optionalParamOK[string](request, "state"); err != nil {
136136
return mcp.NewToolResultError(err.Error()), nil
137137
} else if ok {
138138
update.State = github.Ptr(state)
139139
updateNeeded = true
140140
}
141141

142-
if base, ok, err := optionalParamOk[string](request, "base"); err != nil {
142+
if base, ok, err := optionalParamOK[string](request, "base"); err != nil {
143143
return mcp.NewToolResultError(err.Error()), nil
144144
} else if ok {
145145
update.Base = &github.PullRequestBranch{Ref: github.Ptr(base)}
146146
updateNeeded = true
147147
}
148148

149-
if maintainerCanModify, ok, err := optionalParamOk[bool](request, "maintainer_can_modify"); err != nil {
149+
if maintainerCanModify, ok, err := optionalParamOK[bool](request, "maintainer_can_modify"); err != nil {
150150
return mcp.NewToolResultError(err.Error()), nil
151151
} else if ok {
152152
update.MaintainerCanModify = github.Ptr(maintainerCanModify)

‎pkg/github/server.go‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -113,24 +113,28 @@ func getMe(client *github.Client, t translations.TranslationHelperFunc) (tool mc
113113
}
114114
}
115115

116-
// optionalParamOk is a helper function that can be used to fetch a requested parameter from the request.
116+
// optionalParamOK is a helper function that can be used to fetch a requested parameter from the request.
117117
// It returns the value, a boolean indicating if the parameter was present, and an error if the type is wrong.
118-
func optionalParamOk[T any](r mcp.CallToolRequest, p string) (T, bool, error) {
119-
var zero T
120-
118+
func optionalParamOK[T any](r mcp.CallToolRequest, p string) (value T, ok bool, err error) {
121119
// Check if the parameter is present in the request
122-
val, ok := r.Params.Arguments[p]
123-
if !ok {
124-
return zero, false, nil // Not present, return zero value, false, no error
120+
val, exists := r.Params.Arguments[p]
121+
if !exists {
122+
// Not present, return zero value, false, no error
123+
return
125124
}
126125

127126
// Check if the parameter is of the expected type
128-
typedVal, ok := val.(T)
127+
value, ok = val.(T)
129128
if !ok {
130-
return zero, true, fmt.Errorf("parameter %s is not of type %T, is %T", p, zero, val) // Present but wrong type
129+
// Present but wrong type
130+
err = fmt.Errorf("parameter %s is not of type %T, is %T", p, value, val)
131+
ok = true // Set ok to true because the parameter *was* present, even if wrong type
132+
return
131133
}
132134

133-
return typedVal, true, nil // Present and correct type
135+
// Present and correct type
136+
ok = true
137+
return
134138
}
135139

136140
// isAcceptedError checks if the error is an accepted error.

0 commit comments

Comments
 (0)