Skip to content

Commit aa6151f

Browse files
williammartinwilliammartin
committed
Use arrays rather than comma separated lists
1 parent e16b99a commit aa6151f

File tree

5 files changed

+62
-179
lines changed

5 files changed

+62
-179
lines changed

‎README.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
129129
- `repo`: Repository name (string, required)
130130
- `title`: Issue title (string, required)
131131
- `body`: Issue body content (string, optional)
132-
- `assignees`: Comma-separated list of usernames to assign to this issue (string, optional)
133-
- `labels`: Comma-separated list of labels to apply to this issue (string, optional)
132+
- `assignees`: Usernames to assign to this issue (string[], optional)
133+
- `labels`: Labels to apply to this issue (string[], optional)
134134

135135
- **add_issue_comment** - Add a comment to an issue
136136

@@ -144,7 +144,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
144144
- `owner`: Repository owner (string, required)
145145
- `repo`: Repository name (string, required)
146146
- `state`: Filter by state ('open', 'closed', 'all') (string, optional)
147-
- `labels`: Comma-separated list of labels to filter by (string, optional)
147+
- `labels`: Labels to filter by (string[], optional)
148148
- `sort`: Sort by ('created', 'updated', 'comments') (string, optional)
149149
- `direction`: Sort direction ('asc', 'desc') (string, optional)
150150
- `since`: Filter by date (ISO 8601 timestamp) (string, optional)
@@ -159,8 +159,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
159159
- `title`: New title (string, optional)
160160
- `body`: New description (string, optional)
161161
- `state`: New state ('open' or 'closed') (string, optional)
162-
- `labels`: Comma-separated list of new labels (string, optional)
163-
- `assignees`: Comma-separated list of new assignees (string, optional)
162+
- `labels`: New labels (string[], optional)
163+
- `assignees`: New assignees (string[], optional)
164164
- `milestone`: New milestone number (number, optional)
165165

166166
- **search_issues** - Search for issues and pull requests

‎pkg/github/issues.go‎

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -228,11 +228,21 @@ func createIssue(client *github.Client, t translations.TranslationHelperFunc) (t
228228
mcp.WithString("body",
229229
mcp.Description("Issue body content"),
230230
),
231-
mcp.WithString("assignees",
232-
mcp.Description("Comma-separate list of usernames to assign to this issue"),
233-
),
234-
mcp.WithString("labels",
235-
mcp.Description("Comma-separate list of labels to apply to this issue"),
231+
mcp.WithArray("assignees",
232+
mcp.Description("Usernames to assign to this issue"),
233+
mcp.Items(
234+
map[string]interface{}{
235+
"type": "string",
236+
},
237+
),
238+
),
239+
mcp.WithArray("labels",
240+
mcp.Description("Labels to apply to this issue"),
241+
mcp.Items(
242+
map[string]interface{}{
243+
"type": "string",
244+
},
245+
),
236246
),
237247
),
238248
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
@@ -256,12 +266,13 @@ func createIssue(client *github.Client, t translations.TranslationHelperFunc) (t
256266
}
257267

258268
// Get assignees
259-
assignees, err := optionalCommaSeparatedListParam(request, "assignees")
269+
assignees, err := optionalParam[[]string](request, "assignees")
260270
if err != nil {
261271
return mcp.NewToolResultError(err.Error()), nil
262272
}
273+
263274
// Get labels
264-
labels, err := optionalCommaSeparatedListParam(request, "labels")
275+
labels, err := optionalParam[[]string](request, "labels")
265276
if err != nil {
266277
return mcp.NewToolResultError(err.Error()), nil
267278
}
@@ -311,15 +322,23 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to
311322
),
312323
mcp.WithString("state",
313324
mcp.Description("Filter by state ('open', 'closed', 'all')"),
325+
mcp.Enum("open", "closed", "all"),
314326
),
315-
mcp.WithString("labels",
316-
mcp.Description("Comma-separated list of labels to filter by"),
327+
mcp.WithArray("labels",
328+
mcp.Description("Filter by labels"),
329+
mcp.Items(
330+
map[string]interface{}{
331+
"type": "string",
332+
},
333+
),
317334
),
318335
mcp.WithString("sort",
319336
mcp.Description("Sort by ('created', 'updated', 'comments')"),
337+
mcp.Enum("created", "updated", "comments"),
320338
),
321339
mcp.WithString("direction",
322340
mcp.Description("Sort direction ('asc', 'desc')"),
341+
mcp.Enum("asc", "desc"),
323342
),
324343
mcp.WithString("since",
325344
mcp.Description("Filter by date (ISO 8601 timestamp)"),
@@ -349,10 +368,12 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to
349368
return mcp.NewToolResultError(err.Error()), nil
350369
}
351370

352-
opts.Labels, err = optionalCommaSeparatedListParam(request, "labels")
371+
// Get labels
372+
labels, err := optionalParam[[]string](request, "labels")
353373
if err != nil {
354374
return mcp.NewToolResultError(err.Error()), nil
355375
}
376+
opts.Labels = labels
356377

357378
opts.Sort, err = optionalParam[string](request, "sort")
358379
if err != nil {
@@ -431,12 +452,23 @@ func updateIssue(client *github.Client, t translations.TranslationHelperFunc) (t
431452
),
432453
mcp.WithString("state",
433454
mcp.Description("New state ('open' or 'closed')"),
434-
),
435-
mcp.WithString("labels",
436-
mcp.Description("Comma-separated list of new labels"),
437-
),
438-
mcp.WithString("assignees",
439-
mcp.Description("Comma-separated list of new assignees"),
455+
mcp.Enum("open", "closed"),
456+
),
457+
mcp.WithArray("labels",
458+
mcp.Description("New labels"),
459+
mcp.Items(
460+
map[string]interface{}{
461+
"type": "string",
462+
},
463+
),
464+
),
465+
mcp.WithArray("assignees",
466+
mcp.Description("New assignees"),
467+
mcp.Items(
468+
map[string]interface{}{
469+
"type": "string",
470+
},
471+
),
440472
),
441473
mcp.WithNumber("milestone",
442474
mcp.Description("New milestone number"),
@@ -484,15 +516,17 @@ func updateIssue(client *github.Client, t translations.TranslationHelperFunc) (t
484516
issueRequest.State = github.Ptr(state)
485517
}
486518

487-
labels, err := optionalCommaSeparatedListParam(request, "labels")
519+
// Get labels
520+
labels, err := optionalParam[[]string](request, "labels")
488521
if err != nil {
489522
return mcp.NewToolResultError(err.Error()), nil
490523
}
491524
if len(labels) > 0 {
492525
issueRequest.Labels = &labels
493526
}
494527

495-
assignees, err := optionalCommaSeparatedListParam(request, "assignees")
528+
// Get assignees
529+
assignees, err := optionalParam[[]string](request, "assignees")
496530
if err != nil {
497531
return mcp.NewToolResultError(err.Error()), nil
498532
}

‎pkg/github/issues_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -426,8 +426,8 @@ func Test_CreateIssue(t *testing.T) {
426426
"repo": "repo",
427427
"title": "Test Issue",
428428
"body": "This is a test issue",
429-
"assignees": "user1, user2",
430-
"labels": "bug, help wanted",
429+
"assignees": []string{"user1, user2"},
430+
"labels": []string{"bug", "help wanted"},
431431
},
432432
expectError: false,
433433
expectedIssue: mockIssue,
@@ -615,7 +615,7 @@ func Test_ListIssues(t *testing.T) {
615615
"owner": "owner",
616616
"repo": "repo",
617617
"state": "open",
618-
"labels": "bug,enhancement",
618+
"labels": []string{"bug", "enhancement"},
619619
"sort": "created",
620620
"direction": "desc",
621621
"since": "2023-01-01T00:00:00Z",
@@ -760,8 +760,8 @@ func Test_UpdateIssue(t *testing.T) {
760760
"title": "Updated Issue Title",
761761
"body": "Updated issue description",
762762
"state": "closed",
763-
"labels": "bug,priority",
764-
"assignees": "assignee1,assignee2",
763+
"labels": []string{"bug", "priority"},
764+
"assignees": []string{"assignee1", "assignee2"},
765765
"milestone": float64(5),
766766
},
767767
expectError: false,

‎pkg/github/server.go‎

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"fmt"
88
"io"
99
"net/http"
10-
"strings"
1110

1211
"github.com/github/github-mcp-server/pkg/translations"
1312
"github.com/google/go-github/v69/github"
@@ -119,25 +118,6 @@ func isAcceptedError(err error) bool {
119118
return errors.As(err, &acceptedError)
120119
}
121120

122-
// parseCommaSeparatedList is a helper function that parses a comma-separated list of strings from the input string.
123-
func parseCommaSeparatedList(input string) []string {
124-
if input == "" {
125-
return nil
126-
}
127-
128-
parts := strings.Split(input, ",")
129-
result := make([]string, 0, len(parts))
130-
131-
for _, part := range parts {
132-
trimmed := strings.TrimSpace(part)
133-
if trimmed != "" {
134-
result = append(result, trimmed)
135-
}
136-
}
137-
138-
return result
139-
}
140-
141121
// requiredParam is a helper function that can be used to fetch a requested parameter from the request.
142122
// It does the following checks:
143123
// 1. Checks if the parameter is present in the request.
@@ -221,20 +201,3 @@ func optionalIntParamWithDefault(r mcp.CallToolRequest, p string, d int) (int, e
221201
}
222202
return v, nil
223203
}
224-
225-
// optionalCommaSeparatedListParam is a helper function that can be used to fetch a requested parameter from the request.
226-
// It does the following:
227-
// 1. Checks if the parameter is present in the request, if not, it returns an empty list
228-
// 2. If it is present, it checks if the parameter is of the expected type and uses parseCommaSeparatedList to parse it
229-
// and return the list of strings
230-
func optionalCommaSeparatedListParam(r mcp.CallToolRequest, p string) ([]string, error) {
231-
v, err := optionalParam[string](r, p)
232-
if err != nil {
233-
return []string{}, err
234-
}
235-
l := parseCommaSeparatedList(v)
236-
if len(l) == 0 {
237-
return []string{}, nil
238-
}
239-
return l, nil
240-
}

‎pkg/github/server_test.go‎

Lines changed: 0 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -168,67 +168,6 @@ func Test_IsAcceptedError(t *testing.T) {
168168
}
169169
}
170170

171-
func Test_ParseCommaSeparatedList(t *testing.T) {
172-
tests := []struct {
173-
name string
174-
input string
175-
expected []string
176-
}{
177-
{
178-
name: "simple comma separated values",
179-
input: "one,two,three",
180-
expected: []string{"one", "two", "three"},
181-
},
182-
{
183-
name: "values with spaces",
184-
input: "one, two, three",
185-
expected: []string{"one", "two", "three"},
186-
},
187-
{
188-
name: "values with extra spaces",
189-
input: " one , two , three ",
190-
expected: []string{"one", "two", "three"},
191-
},
192-
{
193-
name: "empty values in between",
194-
input: "one,,three",
195-
expected: []string{"one", "three"},
196-
},
197-
{
198-
name: "only spaces",
199-
input: " , , ",
200-
expected: []string{},
201-
},
202-
{
203-
name: "empty string",
204-
input: "",
205-
expected: nil,
206-
},
207-
{
208-
name: "single value",
209-
input: "one",
210-
expected: []string{"one"},
211-
},
212-
{
213-
name: "trailing comma",
214-
input: "one,two,",
215-
expected: []string{"one", "two"},
216-
},
217-
{
218-
name: "leading comma",
219-
input: ",one,two",
220-
expected: []string{"one", "two"},
221-
},
222-
}
223-
224-
for _, tc := range tests {
225-
t.Run(tc.name, func(t *testing.T) {
226-
result := parseCommaSeparatedList(tc.input)
227-
assert.Equal(t, tc.expected, result)
228-
})
229-
}
230-
}
231-
232171
func Test_RequiredStringParam(t *testing.T) {
233172
tests := []struct {
234173
name string
@@ -492,59 +431,6 @@ func Test_OptionalNumberParamWithDefault(t *testing.T) {
492431
}
493432
}
494433

495-
func Test_OptionalCommaSeparatedListParam(t *testing.T) {
496-
tests := []struct {
497-
name string
498-
params map[string]interface{}
499-
paramName string
500-
expected []string
501-
expectError bool
502-
}{
503-
{
504-
name: "valid comma-separated list",
505-
params: map[string]interface{}{"tags": "one,two,three"},
506-
paramName: "tags",
507-
expected: []string{"one", "two", "three"},
508-
expectError: false,
509-
},
510-
{
511-
name: "empty list",
512-
params: map[string]interface{}{"tags": ""},
513-
paramName: "tags",
514-
expected: []string{},
515-
expectError: false,
516-
},
517-
{
518-
name: "missing parameter",
519-
params: map[string]interface{}{},
520-
paramName: "tags",
521-
expected: []string{},
522-
expectError: false,
523-
},
524-
{
525-
name: "wrong type parameter",
526-
params: map[string]interface{}{"tags": 123},
527-
paramName: "tags",
528-
expected: nil,
529-
expectError: true,
530-
},
531-
}
532-
533-
for _, tc := range tests {
534-
t.Run(tc.name, func(t *testing.T) {
535-
request := createMCPRequest(tc.params)
536-
result, err := optionalCommaSeparatedListParam(request, tc.paramName)
537-
538-
if tc.expectError {
539-
assert.Error(t, err)
540-
} else {
541-
assert.NoError(t, err)
542-
assert.Equal(t, tc.expected, result)
543-
}
544-
})
545-
}
546-
}
547-
548434
func Test_OptionalBooleanParam(t *testing.T) {
549435
tests := []struct {
550436
name string

0 commit comments

Comments
 (0)