-
Notifications
You must be signed in to change notification settings - Fork 558
Add public constructors to McpClient types for reusing cached definitions #938
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: stephentoub <2642209+stephentoub@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.
@copilot, please add a similar ctor (and associated tests) for McpClientPrompt, McpClientResource, and McpClientResourceTemplate. When there's already a relevant test file (as there is for McpClientResourceTemplate, add to the existing file rather than creating a new one).
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…cpClientResourceTemplate Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Added public constructors and tests for McpClientPrompt, McpClientResource, and McpClientResourceTemplate in commit 401e5da. All 29 tests are passing. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
|
||
| _client = client; | ||
| ProtocolTool = tool; | ||
| JsonSerializerOptions = serializerOptions ?? McpJsonUtilities.DefaultOptions; |
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.
Why not use the options from the source McpClientTool?
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 but I'm not following... from where specifically?
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.
Ah yes, my bad. I should've noticed it's Tool and not McpClientTool. Makes perfect sense.
Enables caching tool, prompt, resource, and resource template definitions to avoid redundant
List*Asynccalls on reconnect scenarios.Changes
API Additions
McpClientTool(McpClient client, Tool tool, JsonSerializerOptions? serializerOptions = null)constructorMcpClientPrompt(McpClient client, Prompt prompt)constructorMcpClientResource(McpClient client, Resource resource)constructorMcpClientResourceTemplate(McpClient client, ResourceTemplate resourceTemplate)constructorMcpJsonUtilities.DefaultOptionsTests
Usage
Similar patterns work for prompts, resources, and resource templates using their respective constructors and protocol definitions.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.