Split up tooling utility #1273
Split up tooling utility #1273
Conversation
tonytrg
force-pushed
the
tonytrg/make-toolset-functionality-public
branch
from
64fffaa to
5346e05
Compare
tonytrg
force-pushed
the
tonytrg/make-toolset-functionality-public
branch
from
b2cb714 to
0a0873f
Compare
There was a problem hiding this comment.
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 <[email protected]>
| return result | ||
| } | ||
|
|
||
| func ContainsToolset(tools []string, toCheck string) bool { |
There was a problem hiding this comment.
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.
Or at least CleanToolsets could return a map and an array
There was a problem hiding this comment.
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.
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
Closes:
Splits up the toolset functionality into sub functions, as they were not really reusable.