-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Split up tooling utility #1273
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split up tooling utility #1273
Conversation
64fffaa to
5346e05
Compare
b2cb714 to
0a0873f
Compare
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 refactors toolset utility functions by splitting them from the server package into the github package. The refactoring extracts CleanToolsets, AddDefaultToolset, RemoveToolset, and ContainsToolset as public utility functions, making them reusable across the codebase while simplifying the server initialization logic.
Key changes:
- Moved toolset manipulation utilities from
internal/ghmcp/server.gotopkg/github/tools.goas public functions - Simplified server initialization by composing smaller utility functions instead of one monolithic
cleanToolsetsfunction - Added comprehensive test coverage for the new utility functions in
pkg/github/tools_test.go
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/github/tools.go | Adds four new public utility functions for toolset manipulation: CleanToolsets, AddDefaultToolset, RemoveToolset, and ContainsToolset |
| pkg/github/tools_test.go | Introduces comprehensive test suite covering all new utility functions with various edge cases |
| internal/ghmcp/server.go | Refactors server initialization to use new github package utilities, replacing the monolithic cleanToolsets function |
| internal/ghmcp/server_test.go | Removes tests for the deleted cleanToolsets function (now covered in pkg/github/tools_test.go) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
lgtm!
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.
Can we actually operate on maps everywhere? Right now this function is O(N) because it needs to iterate over all tools in the array, with a map it'd be an O(1). You'd need to refactor CleanToolsets to achieve that
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.
Or at least CleanToolsets could return a map and an array
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.
we are passing lists everywhere, also the libraries expect lists too.
As the tool lists are pretty small we have a neglible performance gain and would have to transform the lists into maps which is more costly probably.
But the biggest hit would be having now to manage transformations between lists and maps everywhere in the codebase where these functions are used, if the utils are maps then they couldnt be used in all the places currently(they are lists everywhere).
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.
https://github.com/github/github-mcp-server/pull/1273/files#diff-efffdb5f6c0e96b376f9f02d7a887ea629058c21a95e161981c5c08e41f12e0cR109 example we would transform it into a map and then back to a list. iterating through the list is the lesser evil
@StefanXhunga
StefanXhunga
left a comment
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.
Hello everyone, thank you for support
@StefanXhunga
StefanXhunga
left a comment
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.
Hello everyone, thank you for support
Uh oh!
There was an error while loading. Please reload this page.
Closes:
Splits up the toolset functionality into sub functions, as they were not really reusable.