Conversation
There was a problem hiding this comment.
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
| "body": "This is a test issue", | ||
| "assignees": "user1, user2", | ||
| "labels": "bug, help wanted", | ||
| "assignees": []string{"user1, user2"}, |
There was a problem hiding this comment.
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"}, |
There was a problem hiding this comment.
Interesting that no test failed.
There was a problem hiding this comment.
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 in bb56733
There was a problem hiding this comment.
I addressed this by adding some new request matching functionality: 33d5752#diff-2e611e3b483afedad4fd22bdbefada38ac15d4d752b7d71e22b454e8e3800b5eR770-R779
33d5752 to
e9bf1f1
Compare
SamMorrowDrums
left a comment
There was a problem hiding this comment.
If this is how things continue, I'm just gonna LGTM the rest! ;-)
e9bf1f1 to
e406c0f
Compare
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.