-
Notifications
You must be signed in to change notification settings - Fork 369
Fix enum serialization #61
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
Fix enum serialization #61
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 fixes enum serialization issues by replacing JsonPropertyName with JsonStringEnumMemberName for the Role and LoggingLevel enums to ensure their JSON output is in lower-case. It also removes the obsolete ToJsonValue extension method from the MCP client, simplifying the request creation for logging level changes.
- Updated the Role enum to use JsonStringEnumMemberName.
- Updated the LoggingLevel enum accordingly.
- Removed the ToJsonValue method from McpClientExtensions and updated its usage.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/ModelContextProtocol/Protocol/Types/Role.cs | Uses JsonStringEnumMemberName for lower-case serialization of Role. |
src/ModelContextProtocol/Protocol/Types/LoggingLevel.cs | Uses JsonStringEnumMemberName for each enum value for consistency. |
src/ModelContextProtocol/Client/McpClientExtensions.cs | Removes the obsolete ToJsonValue conversion in favor of direct enum usage. |
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
Thanks. Can you please add tests? |
5db3040
to
b9964e2
Compare
Added basic serialization tests for enums |
1b0c65d
to
e729a1c
Compare
Added the |
Motivation and Context
Use
JsonStringEnumMemberName
forRole
andLoggingLevel
enum to ensure their serialization is lower-case.Remove
McpClientExtensions.ToJsonValue(LoggingLevel)
since serializing issue is fixed.How Has This Been Tested?
Tested in reproduction for #55.
Breaking Changes
N/A
Types of changes
Checklist
Additional context
N/A