- Notifications
You must be signed in to change notification settings - Fork899
pkg/github: fix use of per page parameter#137
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.
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Tested with code-insiders that list_commits now returns one item. |
pkg/github/search.go Outdated
| @@ -86,7 +86,7 @@ func searchCode(client *github.Client, t translations.TranslationHelperFunc) (to | |||
| mcp.Description("Sort order ('asc' or 'desc')"), | |||
| mcp.Enum("asc", "desc"), | |||
| ), | |||
| mcp.WithNumber("per_page", | |||
| mcp.WithNumber("perPage", | |||
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'm not totally opposed to this, but the reason some of these existed in snake case form was because that'swhat the anthropic server exposed, and initially we just wanted to provide parity before moving forwards.
Obviously I introduced a few silly bugs in doing that last minute 😅
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.
Hm, they actually use a mix:
per_page:https://github.com/modelcontextprotocol/servers/blob/d9f2cb0b1d32acec2d9f1d8d45cf8abaf24791b6/src/github/operations/search.ts#L8perPagehttps://github.com/modelcontextprotocol/servers/blob/d9f2cb0b1d32acec2d9f1d8d45cf8abaf24791b6/src/github/operations/commits.ts#L9
IMO, if that's not crucial it would be better to revert all toper_page because that's what GitHub API uses and to reduce confusion.
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.
Hm, they actually use a mix
Right, so in#90 I changed the tool schemas to match the mix (but messed up the implementation).
I think for the moment let's keep this asperPage, and then we can talk about broader design principles at another time, looking at the entire API surface area. There's definitely a bit of tension between consistency within github-mcp-server, with the JSON-RPC envelope (all camel cased) with the GitHub API (mostly/all snake cased?).
I talked to the team and we're happy to get this in, I'm just going to take a closer look and maybe add some tests to avoid this in future. Thanks!
Thanks for this, let me just run#137 (comment) by the others maintainers. I'd be in favour of it, because as seen, very easy to make mistakes. |
Hey@AlexanderYastrebov, I've pushed a couple of commits on top of your branch to hopefully avoid these kind of shenanigans in the future. Please have a glance and let me know if anything looks surprising to you. Thanks for your contribution! |
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.
Amazing! Thanks both, this is a nice improvement.
AlexanderYastrebov commentedApr 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All good, thank you for the prompt review 🚀 |
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.
Awesome! Thank you everyone for fixing this so quickly! 🚀
Page size tool parameter names were changed to `perPage` withingithub#90while GitHub API uses `per_page` parameter name.This change fixes overlooked inconsistencies.Follow up ongithub#90Follow up ongithub#129Fixesgithub#136Signed-off-by: Alexander Yastrebov <yastrebov.alex@gmail.com>
0f9ef6e intogithub:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Page size tool parameter names were changed to
perPagewithin#90 while GitHub API usesper_pageparameter name.This change fixes overlooked inconsistencies.
Follow up on#90
Follow up on#129
Fixes#136