Skip to content
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 mcp.types.Tool is not following python's naming convention snake_case #113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

azdolinski
Copy link

Renamed the inputSchema field to input_schema in the Tool class to align with Python's naming conventions (snake_case). Added an alias inputSchema for backward compatibility.

Motivation and Context

The name inputSchema does not follow Python's naming convention (PEP 8). This change improves code readability and consistency. Fix related: #97

How Has This Been Tested?

Tested locally by running existing tests.

Breaking Changes

  • Yes. Users will need to update references to the inputSchema field to input_schema. Adding an alias is intended to ease the migration.

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

The addition of the inputSchema alias is intended to provide backward compatibility and facilitate the migration process for users.

propose for fix:
mcp.types.Tool is not following python's naming convention modelcontextprotocol#97
remove Field as it is 2x
@dsp-ant
Copy link
Member

dsp-ant commented Jan 2, 2025

Hmm I think this is sensible, but we need to probably wait for SDK 2.0 to introduce this as it is a BC break. I keep it open for now until we start a 2.0 branch.

@dsp-ant dsp-ant added this to the SDK 2.0.0 milestone Jan 2, 2025
@azdolinski
Copy link
Author

@dsp-ant

OK.. Fully understood.
Just for considered: as alternative which could be not BC.

class Tool(BaseModel):
    """Definition for a tool the client can call."""

    name: str
    """The name of the tool."""
    description: str | None = None
    """A human-readable description of the tool."""
    input_schema: dict[str, Any]
    """A JSON Schema object defining the expected parameters for the tool."""
    model_config = ConfigDict(extra="allow", populate_by_name=True)

    @model_validator(mode='before')
    @classmethod
    def _convert_camel_to_snake_names(cls, data):
        if isinstance(data, dict) and 'inputSchema' in data:
            data['input_schema'] = data.pop('inputSchema')
        return data

The main differences between the original Tool class and this one

  1. Field Naming Convention:

    • In the original version, the field is named inputSchema (camelCase).
    • In the this version, the field is renamed to input_schema (snake_case), which is more Pythonic.
  2. Populate by Name:

    • This version includes populate_by_name=True in the model_config. This allows the model to accept both inputSchema and input_schema as valid field names during initialization.
  3. Model Validator:

    • In this version we adds a @model_validator method (_convert_camel_to_snake_names) to automatically convert inputSchema to input_schema if the input data uses the camelCase version. This ensures backward compatibility with data using the old naming convention.

Example:

# Original version work as:
tool1 = Tool(name="hammer", inputSchema={"type": "object"})

# Proposed version should work in both cases
tool2 = Tool(name="hammer", inputSchema={"type": "object"})  # Still works
tool3 = Tool(name="hammer", input_schema={"type": "object"})  # Also works

In summary, the second version is more flexible and handles both naming conventions seamlessly.
Could be applied also in case of mimeType

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