Skip to content

Make McpJson public to allow flexible protocol development #103

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

Merged
merged 1 commit into from
May 13, 2025

Conversation

parnurzeal
Copy link
Contributor

This MCP repo allows to create new transport from public AbstractTransport but McpJson which is main part for parsing (example) instead being private and not visible to use and block the development of new Transport.

Motivation and Context

I am trying to create a new transport but got blocked on not being able to encode or decode JSONRPCMessage as McpJSON is not public.

How Has This Been Tested?

Open Intellij project and run tests as per this repo guideline.

Breaking Changes

N/A

Types of changes

  • [ X ] New feature (non-breaking change which adds functionality)

Checklist

  • [ X ] I have read the MCP Documentation
  • [ X ] My code follows the repository's style guidelines
  • [ X ] New and existing tests pass locally
  • [ N/A ] I have added appropriate error handling
  • [ N/A ] I have added or updated documentation as needed

@parnurzeal
Copy link
Contributor Author

@e5l Hi Leonid,

My first time to send PR to this rep. Could you help take a look if I need to do any more change?

@e5l
Copy link
Contributor

e5l commented May 12, 2025

Hey @parnurzeal, thank you for the PR. LGTM. Could you run apiDump and commit the change?

@parnurzeal
Copy link
Contributor Author

Thank you @e5l !

Could you guide me a bit how to run apiDump?

@e5l
Copy link
Contributor

e5l commented May 12, 2025

./gradlew apiDump

@parnurzeal
Copy link
Contributor Author

parnurzeal commented May 12, 2025

Thank you! I ran it but no change to it.

Screenshot 2025-05-12 at 3 40 18 PM

@parnurzeal
Copy link
Contributor Author

Does this mean I am fine to submit this? @e5l

@e5l e5l enabled auto-merge (squash) May 12, 2025 08:42
@devcrocod
Copy link
Contributor

@parnurzeal
thanks for the pr!
btw, you can use Json from kotlinx.serialization with your parameters directly

@parnurzeal
Copy link
Contributor Author

Right but I just think that it would be better to use the same McpJson as same as other transport implementation.

@parnurzeal
Copy link
Contributor Author

@e5l I wonder how to get this to merge? it seems auto-merge hasn't tried to merge up til now.

@e5l
Copy link
Contributor

e5l commented May 13, 2025

Sorry for the delay, I'm going to merge it soon

@e5l e5l merged commit 5fa378c into modelcontextprotocol:main May 13, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants