pkg/github: fix use of per page parameter#137
Conversation
|
Tested with code-insiders that list_commits now returns one item. |
pkg/github/search.go
Outdated
| mcp.Enum("asc", "desc"), | ||
| ), | ||
| mcp.WithNumber("per_page", | ||
| mcp.WithNumber("perPage", |
There was a problem hiding this comment.
I'm not totally opposed to this, but the reason some of these existed in snake case form was because that's what 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.
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 to per_page because that's what GitHub API uses and to reduce confusion.
There was a problem hiding this comment.
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 as perPage, 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! |
SamMorrowDrums
left a comment
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Amazing! Thanks both, this is a nice improvement.
All good, thank you for the prompt review 🚀 |
juruen
left a comment
juruen
left a comment
There was a problem hiding this comment.
Awesome! Thank you everyone for fixing this so quickly! 🚀
Page size tool parameter names were changed to `perPage` within github#90 while GitHub API uses `per_page` parameter name. This change fixes overlooked inconsistencies. Follow up on github#90 Follow up on github#129 Fixes github#136 Signed-off-by: Alexander Yastrebov <yastrebov.alex@gmail.com>
williammartin
force-pushed
the
fix-per_page
branch
from
7939297 to
1afbf53
Compare
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