Skip to content

Conversation

pietrozullo
Copy link

Add private method to add tools from tools.

Motivation and Context

There should be a way to add a tool without having to specify a function but constructing the tool directly. This can be handy when one has the inputSchema but not the function. Creating a function given a schema programmatically is not trivial.
The function is a private member just for backward compatibility, probably, the original add_tool should have been add_tool_from_fn.

How Has This Been Tested?

I ran tests.

Breaking Changes

No br.

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

Copy link

@mcp-shadow mcp-shadow bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The PR removes the annotations parameter from the add_tool method signature, which is a breaking change despite the author claiming "No br." (no breaking changes). This could break existing code that passes annotations to add_tool.

  2. The indentation change in the _add_tool method causes a logical error - the return existing is now only executed when warn_on_duplicate_tools is True, changing the behavior of the method.

  3. Private methods should typically be added when there's a clear internal need, but the PR description suggests this is for external usage ("There should be a way to add a tool without..."). If this functionality is needed externally, it should be a proper public API method with tests and documentation.

  4. The PR doesn't include tests demonstrating the specific use case where this would be beneficial.

@ihrpr ihrpr closed this May 13, 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.

2 participants