-
Notifications
You must be signed in to change notification settings - Fork 571
[feat] separate request struct #333
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
[feat] separate request struct #333
Conversation
""" WalkthroughMultiple structs in the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
what is the advantage of this change, and why not change other // GetPromptRequest is used by the client to get a prompt provided by the
// server.
type GetPromptRequest struct {
Request
Params struct {
// The name of the prompt or prompt template.
Name string `json:"name"`
// Arguments to use for templating the prompt.
Arguments map[string]string `json:"arguments,omitempty"`
} `json:"params"`
} |
As far as I know, this change will make calling code simpler CallTool(ctx, mcp.CallToolRequest{
Params: struct {
Name string `json:"name"`
Arguments any `json:"arguments,omitempty"`
Meta *mcp.Meta `json:"_meta,omitempty"`
}{Name: name, Arguments: argument, Meta: nil},
}) after: CallTool(ctx, mcp.CallToolRequestParams{
Name: name,
Arguments: argument,
Meta: nil,
},) As for other requests, I'm sorry that I forgot to change that. |
@OswinWu It would be great if you could include other |
sure |
I have processed other requests. Please let me know if there's any request I forgot to change. |
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.
thanks for your update, I have some concerns:
- for so many structs with one or two fields, anonymous nested struct seems to be better choice, because anonymous nested struct seems more clear and new struct will not be used by other struct
- the 'xxxRequestParams' naming style seems strange.
From my point of view, this change will be easier to use, as mentioned in this issue. As for the naming style, I haven't come up with other thoughts😢. Do you have any other great ideas? |
thanks for you present more details. as for the naming style, how about |
Sure, that's a good idea! |
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. Seems like there are more places where we use anonymous structs. Maybe we can also change that in other places. It can be a separate PR.
Description
Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
This PR aims to avoid anonymous struct
Summary by CodeRabbit