Non-Idiomatic Naming

[virtualritz]

Naming in this crate does not conform to official Rust API guidelines on naming.

Particularly the MCP prefix of some types should read Mcp (e.g. McpServer) and getters should not start with get_.

Vice versa, I think JsonrpcErrorError should be JsonRpcError etc. but that may be because this stuff is generated? Maybe a type alias in the resp. crate would be a good idea here.

type JsonRpcError = JsonrpcErrorError;

Would you accept a PR that fixes those?

I can add aliases for the old types/method names w. deprecation warnings too, if you want. Given that this crate does not have that many users (yet) when looking at stats on crates.io it may not be be necessary and a changelog note may be enough. 😁

[hashemix]

Hey @virtualritz ,

Thank you for your suggestion! I truly appreciate your contribution, and any further input is always welcome.

Let's break this down a bit to clarify everything and ensure we're all on the same page:

MCP prefix of some types should read Mcp, getters should not start with get_

Totally valid point 👍 , I would be happy to review a PR fixing those naming ✔

Regarding the JsonRpcError :

I think JsonrpcErrorError should be JsonRpcError etc. but that may be because this stuff is generated? Maybe a type alias in the resp. crate would be a good idea here.

That requires some clarification from my end.

JsonrpcError and JsonrpcErrorError are not exactly the same. we already have the JsonrpcError struct that matches official MCP schema name.

JsonrpcError is the main struct that contains error, requestId and jsonrpc.
JsonrpcErrorError is the inner error struct that constructs a JsonrpcError.
because it is auto generated, it has the struct name as prefix : JsonrpcError + Error = JsonrpcErrorError.

In the handler methods, we return JsonrpcErrorError so that developers don't have to worry about setting the requestId.

That said, I can understand how it might be a bit confusing. Perhaps we could choose a better name for it and use a type alias. However, it shouldn't be JsonrpcError.

[virtualritz]

Maybe I am missing something but what about simplifying those two into a single enum where the data is carried by the members?

JsonRpcErrorPayload {
    request_id: RequestId,
    message: String,
    data: Option<Value>,
}

enum JsonRpcError {
    ParseError(JsonRpcErrorPayload),
    InvalidRequest(JsonRpcErrorPayload),
    MethodNotFound(JsonRpcErrorPayload),
    InvalidParameters(JsonRpcErrorPayload),
    InternalError(JsonRpcErrorPayload),
} 

impl From<JsonRpcError> for i64 {
    fn from(error: JsonRpcError) -> i64 {
        match error {
            JsonRpcError::ParseError(_) => -32_700,
            JsonRpcError::InvalidRequest(_) => -32_600,
            JsonRpcError::MethodNotFound(_) => -32_601,
            JsonRpcError::InvalidParameters(_) => -32_602,
            JsonRpcError::InternalError(_) => -32_603,
        }
    }
}

but maybe I'm missing something?

The above aside it seems weird that the JsonrpcErrorError error has all members public and then has methods to set these members. I.e. if new() only allows RpcErrorCodes. The latter IMHO should be a struct with UPPER_CASE constants or an enum with CamelCase variants and be called RpcErrorCode then (singular). It's a non-idiomatic mix of those two atm.

I.e. nothing stops me from doing:

let error = JsonrpcErrorError {
     code: 42,
     message: "Something went wrong",
     data: None,
};

42 is not a valid code from RpcErrorCodes that JsonrpcErrorError::new() would accept. Just saying: if the above is illegal, maybe the JsonrpcErrorError members should be made private?

The advantage of them being public though is mainly because that argument names are not visible at call sites in Rust.

The methods that construct specific errors make sense but new(), and with_...() variants don't. I.e. I would derive Default for all public types in the schema where this is possible anyway and then you can use struct-init shorthand where it is clear what is happening.

Ah yeah, I also forgot about crate naming above; here is a quote from the above link about API naming guidelines:

Crate names should not use -rs or -rust as a suffix or prefix. Every crate is Rust! It serves no purpose to remind users of this constantly.

I.e. I'd consider re-publishing these crates under a different names omitting the rust- prefix. 😁.

[hashemix]

what about simplifying those two into a single enum where the data is carried by the members?

I agree for that case enum could be a more clean approach. Currently, JSONRPCError will be parsed and converted to a struct rather than a enum and I added error codes and impl to the generated struct in shema_utils.rs. I am trying to avoid modifying or customizing the auto generated schema, but for this one I may adjust the code generator to make an exception .

The above aside it seems weird that the JsonrpcErrorError error has all members public and then has methods to set these members. I.e. if new() only allows RpcErrorCodes.

Yeah the idea was to keep the schema.rs as is generated with zero modification (and have my opinionated implementations in schema_utils.rs which developers could opt out) but I may reconsider that for this case 👍

By the way this repo is not the right place to discuss rust-mcp-schema, I will create a couple of issues in there to consider some of the suggestions..

lets keep it related to Rust naming convention and your PR,

[hashemix]

Thank you for raising these thoughtful points and for your contributions.
I am closing this issue as the primary concerns are addressed in #8 or resolved as follows:

1- ✅ Idiomatic Naming for Traits and Getters
This has been addressed in #8

2- ✅ Friendlier Name for JsonrpcErrorError
The auto-generated struct name was updated to RpcError for clarity in rust-mcp-schema PR #65.

3- 🔄 Using Enum Instead of Struct for JsonRpcError
While I agree with that, My goal is to align with the official schema, which currently uses a struct and const for error codes.
Should the upstream schema adopt an enum, our generated schema will automatically reflect this change.

4- ❌ Republishing Crates Without the rust- Prefix
After consideration, I decided not to pursue renaming the crates at this time.