Skip to content

Conversation

@Kludex
Copy link
Member

@Kludex Kludex commented Jan 25, 2026

No description provided.

@claude
Copy link

claude bot commented Jan 25, 2026

Code Review

I found several CLAUDE.md compliance issues related to missing docstrings on public API methods.

Issue: Missing Docstrings on Public API Methods

The MCPError class is missing docstrings on several public API methods and properties. According to CLAUDE.md:

Code Quality

  • Public APIs must have docstrings

Since MCPError is exported in __all__, it's a public API and all its public methods require docstrings.

Missing docstrings in src/mcp/shared/exceptions.py:

  1. __init__ method (line 13) - The previous implementation had a docstring, but it was removed in this refactor
  2. code property (line 18) - New property accessor needs docstring
  3. message property (line 22) - New property accessor needs docstring
  4. data property (line 26) - New property accessor needs docstring
  5. from_jsonrpc_error classmethod (line 30) - New factory method needs docstring
  6. from_error_data classmethod (line 34) - New factory method needs docstring

For reference, the UrlElicitationRequiredError class in the same file properly documents all its public methods and properties with docstrings.

Suggested Fix

Add docstrings to all public methods and properties. Example:

def __init__(self, code: int, message: str, data: Any = None):
    """Initialize MCPError.
    
    Args:
        code: Error code from MCP protocol
        message: Human-readable error message
        data: Optional additional error data
    """
    self.error = ErrorData(code=code, message=message, data=data)

@property
def code(self) -> int:
    """The error code from the MCP protocol."""
    return self.error.code

No bugs were found in the implementation - the refactoring logic is sound.

@Kludex Kludex force-pushed the refactor-mcp-error branch from 690434d to 1e3e62f Compare January 25, 2026 18:35
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