-
Notifications
You must be signed in to change notification settings - Fork 1.5k
list changed handlers on client constructor #1206
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
Conversation
commit: |
|
See we've extended this PR to add the same functionality for prompts and resources changes. 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.
@chipgpt's PR was also correlating the server capabilities - tools.listChanged = true.
Likely we should demonstrate this here for tools, resources and prompts listChanged.
Should we also add some checks? It feels odd to allow Clients to listen for the change notifications if the server did not advertise this capability (or explicitly advertised it as listChanged: false?
|
adding checks |
Add tests verifying that listChanged handlers are only activated when the server advertises the corresponding capability. Tests cover: - Handler not activated when server lacks listChanged capability - Handler activated when server advertises capability - No handlers activated when server has no listChanged capabilities - Partial capability support with some handlers activated Also defer handler setup until after initialization when server capabilities are known,
KKonstantinov
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.
LGTM!
WIP api design.
Builds off work done by @chipgpt #1068
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context