Introduce acceptance helpers to ElicitResult and client capability checks on IMcpServer #666
Conversation
| { | ||
| Throw.IfNull(result); | ||
| return string.Equals(result.Action, "cancel", StringComparison.OrdinalIgnoreCase); | ||
| } |
There was a problem hiding this comment.
If we want to do this, why not have them as JsonIgnore properties directly on ElicitResult?
There was a problem hiding this comment.
Great point. While I was writing SupportsElicitation method, I biased to use extension method. Having properties with JsonIgnore would makes more sense. I'll refactor it.
There was a problem hiding this comment.
@stephentoub I've completed refactoring with your eye-opening question. Do you think this PR contributes to developer experience and you'd like to see it in the SDK?
There was a problem hiding this comment.
Thanks. We'll discuss it.
dogukandemir
changed the title
There was a problem hiding this comment.
I'm personally a fan of IsAccepted/IsDeclined/IsCanceled so you don't have to carefully read the doc comments or look up the spec to see what the possible values are. An alternative might be an enum, but this is certainly more flexible.
If we add SupportsElicitation, I think we should add SupportsSampling and SupportsRoots for completeness.
| /// <remarks> | ||
| /// When <see langword="true"/>, the server can call <see cref="McpServerExtensions.ElicitAsync"/> to request additional information from the user via the client. | ||
| /// </remarks> | ||
| public static bool SupportsElicitation(this IMcpServer server) |
There was a problem hiding this comment.
Maybe ClientSupportsElicitation? The convenience of not going through client capabilities is reasonable, but it feels weird to have a client capability be a direct pseudo-property on IMcpServer with the client link being entirely implicit.
There was a problem hiding this comment.
renamed with ClientSupportsElicitation
| Throw.IfNull(server); | ||
| return server.ClientCapabilities?.Elicitation is not null; | ||
| } | ||
|
|
||
| private static void ThrowIfSamplingUnsupported(IMcpServer server) |
There was a problem hiding this comment.
Same as above. Having the Client word would reduce risk of developer confusion.
@halter73 Checking support of sampling and roots extension methods are added with ClientSupportsSampling and ClientSupportsRoots method names. Client prefix is suggested by @PederHP and it totally makes sense in this context. |
dogukandemir
changed the title
…potential string comparison errors
rename SupportsElicitation to ClientSupportsElicitation
dogukandemir
force-pushed
the
feature/elicitation-extension-methods-to-improve-readability
branch
from
213cbe9 to
9c6314f
Compare
This PR has following additions on IMcpServer and ElicitResult.
Motivation and Context
Motivation is to make code more readable and less error-prone than direct string comparisons.
How Has This Been Tested?
All newly added functionality tested in unit tests.
Breaking Changes
There is no breaking change.
Types of changes
Checklist
Additional context