Skip to content

Extend modules instead of inheriting classes #48

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

Closed
wants to merge 1 commit into from

Conversation

sambostock
Copy link
Contributor

Motivation and Context

Classes are meant to be instantiated, but we expect consumers to use "macros" and define a singleton call method, so we should have them extend a Module instead.

How Has This Been Tested?

The tests have been updated.

Breaking Changes

Consumers will need to update their tools and prompts to use the module based approach instead of the class based approach. We could facilitate this by providing an autocorrecting RuboCop cop.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

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

Additional context

Classes are meant to be instantiated, but we expect consumers to use
"macros" and define a singleton `call` method, so we should have them
`extend` a `Module` instead.
@kfischer-okarin
Copy link
Contributor

kfischer-okarin commented Jun 6, 2025

I'm also not happy with the current design, but I'm unsure if this change is the best way forward.

I think one good way to design libraries/code is to not surprise/confuse the user... and from my point of view the main surprise of the current design has not been resolved:

When I see a class defined, I expect to create instances and use them somehow.

But that's not how Tool/Prompt definition works right now. The class objects are singletons that contain tool definition information and handle the conversion to the MCP JSON response format for you. Nothing useful can happen by instantiating them.

Btw: That criticism applies only to the class MyTool < MCP::Tool style of definition - tool = MCP::Tool.define is totally fine in that regard since it hides away the construction behind a DSL. It would have been fine if it had just been the DSL opaquely constructing the convenience singleton object - but additionally adding the class inheritance based version makes it more confusing imo.

If I were to improve the design I would probably introduce a plain Ruby Object ToolDefinition class (could most likely even be a Data class) holding name, description, input_schema, annotations, handler attributes, and providing a to_h (and I guess a convenience call method calling the handler instead of requiring .handler.call). This would also make the interface of the definition container objects explicit in code.

And then refactor the current Tool.define DSL to internally construct such an object.

Same for Prompt (maybe with a build instance method calling the handler)

For some reason, Resource and ResourceTemplate already have an instance-based design - maybe they could also be renamed to ResourceDefinition for consistency 🤔

@sambostock
Copy link
Contributor Author

Thanks for the feedback @kfischer-okarin! I'll explore the _Definition idea.

@sambostock
Copy link
Contributor Author

@kfischer-okarin I've opened #56 as a draft to get feedback on a more "definition" like approach.

@koic
Copy link
Member

koic commented Jun 10, 2025

What benefits would this provide to users?

There may be debate over whether an inheritance-based approach is preferable, but if an API incompatibility is introduced, users should receive benefits that justify the cost of the change.

Additionally, the migration path for an API change should follow this approach:

  1. Display a deprecation warning to avoid breaking existing code
  2. Remove support for the old code in the future (breaking change)

RuboCop is a supporting tool, and as a product, this kind of migration flow is user-friendly.

@sambostock
Copy link
Contributor Author

@koic I think I will close this PR, as I prefer the approach in #56. However, your comment is worth answering, so I will copy it there and answer there (as the answers would be different between #48 & #56).

@sambostock sambostock closed this Jun 10, 2025
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