Decouple Handlers and Collections from protocol types #765
Conversation
jozkee
force-pushed
the
capabilities4
branch
from
78218a4 to
82b62cf
Compare
| /// See the <see href="https://github.com/modelcontextprotocol/specification/blob/main/schema/">schema</see> for details. | ||
| /// </para> | ||
| /// </remarks> | ||
| public sealed class CompletionsCapability |
There was a problem hiding this comment.
Why do we need to keep this empty class?
There was a problem hiding this comment.
Ah, I now notice the remark about it being empty in the spec. Could you check that this is still the case with the latest revision and whether keeping this around is still necessary?
There was a problem hiding this comment.
Doesn't the presence of an instance of this signal to the connected party whether that capability is available?
src/ModelContextProtocol.Core/Protocol/ElicitationCapability.cs
Outdated
Show resolved
Hide resolved
…ion) from Capabilities by promoting them to the options types. Also, group all handlers in container classes, for server, I decided to move McpServerHandlers down to Mcp.Core and move the NotificationHandlers there too. Also, Fix ListResourceTemplatesHandler issue.
jozkee
force-pushed
the
capabilities4
branch
from
180dcd0 to
0a0ff59
Compare
| /// </remarks> | ||
| [JsonIgnore] | ||
| public McpRequestHandler<CompleteRequestParams, CompleteResult>? CompleteHandler { get; set; } | ||
| [Obsolete($"Use {nameof(McpServerHandlers.CompleteHandler)} instead.")] |
There was a problem hiding this comment.
You might want to state the full path for McpServerOptions.Handlers.CompleteHandler for easier discoverability.
| /// then be unregistered by disposing of the <see cref="IAsyncDisposable"/> returned from the method. | ||
| /// </para> | ||
| /// </remarks> | ||
| public IEnumerable<KeyValuePair<string, Func<JsonRpcNotification, CancellationToken, ValueTask>>>? NotificationHandlers { get; set; } |
There was a problem hiding this comment.
Shouldn't this be a Dictionary<string, Func<...>> for easier initialization syntax? cc @stephentoub
There was a problem hiding this comment.
Note that initializer expressions work even if we don't provide a setter. This should ensure that we always use a consistent key comparer.
| var notificationHandlers = new NotificationHandlers(); | ||
| var requestHandlers = new RequestHandlers(); | ||
|
|
||
| if (options.Capabilities is { } capabilities) |
There was a problem hiding this comment.
Related to this discussion, does removing the check on the capability make this diverge from the MCP spec?
| /// </remarks> | ||
| [JsonIgnore] | ||
| public McpRequestHandler<GetPromptRequestParams, GetPromptResult>? GetPromptHandler { get; set; } | ||
| [Obsolete($"Use {nameof(McpServerHandlers.GetPromptHandler)} instead.")] |
There was a problem hiding this comment.
Please add a comment to all obsoleted members linking to #774 for easier removal in the future.
Decouple Handlers and Collections (e.g. ToolsHandler and ToolsCollection) from Capabilities by promoting them to the options types.
Also, group all handlers in container classes, for server, I decided to move McpServerHandlers down to Mcp.Core and move the NotificationHandlers there too.