- Notifications
You must be signed in to change notification settings - Fork899
Use arrays rather than CSV#82
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 updates the issue creation, listing, and updating logic to use arrays (string[]) instead of comma-separated strings, addressing#31.
- Removed CSV parsing helper functions and related tests.
- Updated parameter definitions and test data in issues and server modules to accept arrays.
- Updated README documentation to reflect the new API parameter types.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/server_test.go | Removed CSV parsing tests in favor of array parameter usage |
| pkg/github/server.go | Removed CSV parsing helper functions and updated parameter handling |
| pkg/github/issues_test.go | Updated tests to send arrays for assignees, labels, and filter parameters |
| pkg/github/issues.go | Changed parameter definitions from strings to arrays for labels/assignees |
| README.md | Updated parameter descriptions to indicate arrays instead of CSV strings |
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused.Learn more
pkg/github/issues_test.go Outdated
| @@ -426,8 +426,8 @@ func Test_CreateIssue(t *testing.T) { | |||
| "repo": "repo", | |||
| "title": "Test Issue", | |||
| "body": "This is a test issue", | |||
| "assignees": "user1, user2", | |||
| "labels": "bug, help wanted", | |||
| "assignees": []string{"user1, user2"}, | |||
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 test input for the 'assignees' parameter is still a single string containing a comma-separated list instead of two separate array elements. Consider updating it to []string{"user1", "user2"} to reflect the new array format.
| "assignees": []string{"user1,user2"}, | |
| "assignees": []string{"user1","user2"}, |
Copilot uses AI. Check for mistakes.
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.
Interesting that no test 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.
Right, it's a bit weird that we don't actually assert that the handler provided the correct arguments to the API:
github-mcp-server/pkg/github/issues_test.go
Lines 419 to 422 inbb56733
| mock.WithRequestMatchHandler( | |
| mock.PostReposIssuesByOwnerByRepo, | |
| mockResponse(t,http.StatusCreated,mockIssue), | |
| ), |
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.
I addressed this by adding some new request matching functionality:33d5752#diff-2e611e3b483afedad4fd22bdbefada38ac15d4d752b7d71e22b454e8e3800b5eR770-R779
33d5752 toe9bf1f1CompareThere 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.
If this is how things continue, I'm just gonna LGTM the rest! ;-)
e9bf1f1 toe406c0fCompared01e705 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Description
Fixes#31
Reviewer Notes
I added a new testing pattern for the tool implementations I changed because there was no safety around the outgoing requests being correct. I didn't roll it out everywhere because it's a big lift and a pattern I would want feedback on.
The README and description of these arrays isn't great, but I tried to follow the previous format.