-
Notifications
You must be signed in to change notification settings - Fork 250
feat: unified error type #308
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
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.
Pull Request Overview
This PR unifies error handling by introducing a central RmcpError
and a dynamic transport error type, DynamicTransportError
, removing former generic error parameters and redundant bounds.
- Created
RmcpError
for all service, initialization, runtime, and transport-creation errors - Replaced generic
ClientInitializeError<E>
/ServerInitializeError<E>
with concrete types usingDynamicTransportError
- Updated examples, tests, service modules, handlers, and macros to use
ErrorData
/RmcpError
and removed oldError
generics
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
examples/servers/src/common/counter.rs | Swapped alias Error as McpError → ErrorData as McpError |
examples/clients/src/git_stdio.rs | Changed main to return Result<(), RmcpError> & added map_err |
crates/rmcp/tests/test_progress_subscriber.rs | Updated return type from Error → ErrorData |
crates/rmcp/tests/test_notification.rs | Updated return type from Error → ErrorData |
crates/rmcp/tests/test_complex_schema.rs | Swapped Error as McpError → ErrorData as McpError |
crates/rmcp/tests/common/handlers.rs | Swapped Error as McpError → ErrorData as McpError |
crates/rmcp/src/transport.rs | Added Transport::name & DynamicTransportError type |
crates/rmcp/src/service/tower.rs | Switched S::Error: Into<Error> → Into<ErrorData> |
crates/rmcp/src/service/server.rs | Removed generic ServerInitializeError<E> , added helper |
crates/rmcp/src/service/client.rs | Removed generic ClientInitializeError<E> , added helper |
crates/rmcp/src/service.rs | Replaced Error generics, updated ServiceError variant |
crates/rmcp/src/model/content.rs | Changed json methods to return ErrorData |
crates/rmcp/src/lib.rs | Re-exported Error , ErrorData , RmcpError , deprecated old alias |
crates/rmcp/src/handler/server/tool.rs | Updated parse_json_object & traits to use ErrorData |
crates/rmcp/src/handler/server/router/tool.rs | Updated return types to ErrorData |
crates/rmcp/src/handler/server/router.rs | Updated handler signatures to ErrorData |
crates/rmcp/src/handler/server.rs | Swapped import Error as McpError → ErrorData as McpError |
crates/rmcp/src/handler/client.rs | Swapped import Error as McpError → ErrorData as McpError |
crates/rmcp/src/error.rs | Added RmcpError enum and transport_creation helper |
crates/rmcp/README.md | Updated example to use ErrorData as McpError |
crates/rmcp-macros/src/tool_handler.rs | Updated generated handlers to return ErrorData |
crates/rmcp-macros/src/lib.rs | Updated doc comments to rmcp::ErrorData |
crates/rmcp-macros/README.md | Updated macro examples to use ErrorData |
Comments suppressed due to low confidence (4)
examples/servers/src/common/counter.rs:5
- [nitpick] Aliasing
ErrorData
asMcpError
can be misleading; consider using a direct alias likeErrorData
or the unifiedRmcpError
for clarity.
ErrorData as McpError, RoleServer, ServerHandler,
crates/rmcp/src/transport.rs:248
- Consider adding unit tests for
DynamicTransportError::is
anddowncast
methods to ensure transport error conversion works as expected.
#[derive(Debug, thiserror::Error)]
crates/rmcp/README.md:18
- [nitpick] Update this example to use the new
RmcpError
alias instead ofErrorData
for consistency with the unified error type.
use rmcp::{ErrorData as McpError, ServiceExt, model::*, tool, tool_router, transport::stdio, handler::server::tool::ToolCallContext, handler::server::router::tool::ToolRouter};
crates/rmcp/src/lib.rs:93
- [nitpick] Review whether the
#[allow(deprecated)]
on the re-export ofError
is still necessary and remove it if the deprecation period is complete.
#[allow(deprecated)]
@@ -48,7 +48,6 @@ pub enum ServiceError { | |||
Timeout { timeout: Duration }, | |||
} |
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.
This empty impl ServiceError {}
block is redundant and can be removed to reduce clutter.
Copilot uses AI. Check for mistakes.
conflict |
@jokemanfire resolved |
cc @ssddOnTop please check this |
Thanks @4t145 I'll review this in a bit |
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.
There's some overall feedback at #299 (comment)
But the PR itself looks good to me.
Provide a unified error type.
RmcpError
, which can be converted from every possible error type when you create and operate a rmcp service.ClientInitializeError
andServerInitializeError
, and use a new type calledDynamicTransportError
, witch include the transport type name.ServiceError
useDynamicTransportError
too.From<std::io::Error>
bound.Motivation and Context
#299
How Has This Been Tested?
Breaking Changes
There is a little breaking change, I cancelled the error generic type in
InitializeError
. User can still fetch the inner transport error by usingDynamicTransportError::downcast
andDynamicTransportError::is
. If user didn't match the inner type before, that would not be a problem.Types of changes
Checklist
Additional context
This change could make user confuse about the
rmcp::Error
andrmcp::RmcpError
, so I added an deprecate marker onrmcp::Error
.And in 0.3.x,
rmcp::RmcpError
would become thermcp::Error
. The current rmcp::Error would becomermcp::ErrorData
.