- Notifications
You must be signed in to change notification settings - Fork1k
chore: separate toolset creation from init and use typed error#487
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.
Pull Request Overview
This PR introduces a typed error for missing toolsets and decouples toolset creation from enabling so callers can compose and enable toolsets separately.
- Add
ToolsetDoesNotExistError, replacefmt.Errorfwith the new error type, and implementGetToolset(). - Update tests to assert against the typed error and add new coverage for
GetToolset. - Rename
InitToolsetstoDefaultToolsetGroupin GitHub integration and move enabling logic into the server initialization.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/toolsets/toolsets.go | AddedToolsetDoesNotExistError, newGetToolset, replaced error strings |
| pkg/toolsets/toolsets_test.go | Updated tests to useerrors.Is against the typed error, addedTestToolsetGroup_GetToolset |
| pkg/github/tools.go | RenamedInitToolsets →DefaultToolsetGroup, removed inline enabling |
| internal/ghmcp/server.go | Changed server init to callEnableToolsets after creating the group |
Comments suppressed due to low confidence (2)
pkg/github/tools.go:18
- [nitpick] Consider renaming
DefaultToolsetGrouptoNewDefaultToolsetGrouporInitDefaultToolsetGroupto follow Go constructor naming conventions and clarify intent.
func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) *toolsets.ToolsetGroup {pkg/toolsets/toolsets.go:15
- The code calls
fmt.Sprintfbutfmtis not imported; add"fmt"to the import block to avoid a compilation error.
return fmt.Sprintf("toolset %s does not exist", e.Name)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.
👍🏻
c17ebfe intomainUh oh!
There was an error while loading.Please reload this page.
This PR makes it easier to identify the error when a toolset does not exist, and also separates toolset creation from initialization, so it's possible to compose toolsets in more ways.