-
Notifications
You must be signed in to change notification settings - Fork 546
refactor: reformat McpSchema.java and reorganize imports #392
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
Conversation
- Remove unused imports (CallToolResult, TextContent) - Standardize code formatting and indentation throughout - Adjust formatter comments positioning - Improve readability of record definitions and builder classes Signed-off-by: Christian Tzolov <[email protected]>
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.
I think we should standardize the spacing before formatter comments across the file as well. Couldn't confirm that each instance of formatting was correct by taking a look but looks good for the most part. 1 question: Did we use ./mvnw spring-javaformat:apply
to fix formatting or was it manual effort?
@JsonProperty("jsonrpc") String jsonrpc, | ||
@JsonProperty("method") String method, | ||
@JsonProperty("id") Object id, | ||
@JsonProperty("params") Object params) implements JSONRPCMessage { // @formatter:on |
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.
Nit: Let's standardize either having a space or not before we write formatter on/off.
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.
Good point
Signed-off-by: Christian Tzolov <[email protected]>
Thanks for the review @pantanurag555
I did it by hand. But run the javaformat:apply after I moved the Also, perhaps we can improve our format rules to remove trailing empty lines and add better rules for the import order and maybe prevent wildcard imports. Not sure how much of this is possible. |
Signed-off-by: Christian Tzolov <[email protected]>
This enhances API documentation and developer experience when working with the MCP Java SDK schema definitions. Signed-off-by: Christian Tzolov <[email protected]>
@tzolov |
What are the |
The |
I had a look at the spring-javaformat repo to see if there was documentation on this, but it looks like it isn't super customizable on its own. Doing any meaningful customization requires also adding Checkstyle and the Checkstyle plugin, it looks like. Worth doing, but not a priority as I see it either, then. We have PRs waiting on this already, so something to revisit later. It'd be useful to at least set up a |
@LucaButBoring I agree that this needs to be addressed in a separate PR. Here is the ticket: #399 |
rebased, squashed and merged at 8d71b76b21ec9179a1c4644710af08f2a33410cd |
- Remove unused imports (CallToolResult, TextContent) - Standardize code formatting and indentation throughout - Adjust formatter comments positioning - Improve readability of record definitions and builder classes - Add comprehensive JavaDoc documentation to McpSchema Signed-off-by: Christian Tzolov <[email protected]>
Reformat McpSchema.java and reorganize imports for improved code consistency and readability
Motivation and Context
This change improves code maintainability and consistency by:
How Has This Been Tested?
Breaking Changes
No breaking changes - this is purely a formatting and import organization change with no functional modifications.
Types of changes
Checklist
Additional context
This refactoring focuses solely on code style and organization:
@formatter:off/on
comment positioning for better code structure