Raise error if method is missing in either parent or subclasse; add tests for mixin #95
Conversation
Reviewer's Guide by SourceryThe pull request enhances the InterfaceProtocolCheckMixin to ensure that methods are correctly implemented in both parent and subclass, raising errors if methods are missing. Additionally, it adds comprehensive tests to validate the mixin's behavior in various scenarios. Sequence diagram for InterfaceProtocolCheckMixin method validationsequenceDiagram
participant S as Subclass
participant M as InterfaceProtocolCheckMixin
participant P as ParentClass
S->>M: __init_subclass__()
activate M
M->>M: Get defined methods
M->>P: Check if method exists
alt Method missing in parent
M-->>S: Raise NotImplementedError
else Method exists
M->>M: Compare method signatures
alt Signatures don't match
M-->>S: Raise NotImplementedError
else Signatures match
M-->>S: Continue initialization
end
end
deactivate M
Class diagram for InterfaceProtocolCheckMixin implementationclassDiagram
class InterfaceProtocolCheckMixin {
+__init_subclass__()
}
class ParentClass {
+method()
}
class SubClass {
+method()
}
ParentClass --|> InterfaceProtocolCheckMixin
SubClass --|> ParentClass
note for InterfaceProtocolCheckMixin "Validates method implementations
and signatures between
parent and subclass"
Flow diagram for method validation processgraph TD
A[Start Validation] --> B{Method in Parent?}
B -->|No| C[Raise Error:
Missing in Parent]
B -->|Yes| D{Method in Subclass?}
D -->|No| E[Raise Error:
Missing Override]
D -->|Yes| F{Check Signatures}
F -->|Match| G[Continue]
F -->|Mismatch| H[Raise Error:
Signature Mismatch]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ugly-custard - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Review instructions: 1 issue found
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Got: {subclass_method_params.keys()} | ||
| """) | ||
| for cls_signature, subclass_signature in zip( | ||
| cls_method_params.items(), subclass_method_params.items() | ||
| for parent_cls_signature, subclass_signature in zip( |
There was a problem hiding this comment.
issue: Parameter comparison using zip() assumes parameters are in the same order, which might not catch valid implementations with reordered parameters
Consider using a more flexible comparison that matches parameters by name rather than position to allow valid parameter reordering in subclass implementations.
| @@ -49,21 +49,36 @@ def __init_subclass__(cls, **kwargs): | |||
| # raise Exception(inspect.getmembers(cls, predicate=inspect.isfunction)) | |||
| for defined_method in ( | |||
| method_name | |||
| for method_name, _ in inspect.getmembers(cls, predicate=inspect.ismethod) | |||
| for method_name, _ in inspect.getmembers(cls, predicate=inspect.isfunction) | |||
There was a problem hiding this comment.
issue: Using isfunction might not properly handle all method types like static and class methods
Consider adding specific handling for @staticmethod and @classmethod decorators to ensure proper method detection and comparison.
| # A correct subclass implementation | ||
| class CorrectImplementation(ParentInterface, InterfaceProtocolCheckMixin): | ||
| def method_one(self, param: int) -> str: | ||
| return str(param) |
There was a problem hiding this comment.
suggestion (testing): Add test for missing first method
The test only checks for missing second method. Consider adding another test case where method_one is missing to ensure both scenarios are covered.
def test_missing_methods():
with pytest.raises(
NotImplementedError,
match="Subclass 'MissingMethodImplementation' must override the method 'method_two' from the parent class 'ParentInterface'.",
):
class MissingMethodImplementation(ParentInterface, InterfaceProtocolCheckMixin):
def method_one(self, param: int) -> str:
return str(param)
with pytest.raises(
NotImplementedError,
match="Subclass 'MissingFirstMethodImplementation' must override the method 'method_one' from the parent class 'ParentInterface'.",
):
class MissingFirstMethodImplementation(ParentInterface, InterfaceProtocolCheckMixin):
def method_two(self) -> int:
return 42| class ParentInterface: | ||
| def method_one(self, param: int) -> str: | ||
| """Method one of Base Parent Interface Class""" | ||
| raise NotImplementedError("Method one not implemented") | ||
|
|
||
| def method_two(self, value: str) -> int: | ||
| """Method two Base Parent Interface Class""" | ||
| raise NotImplementedError("Method two not implemented") |
There was a problem hiding this comment.
suggestion (testing): Add test for method signature mismatch
The changes in mixins.py include parameter type checking, but there are no tests for mismatched parameter types or return types. Consider adding tests where the subclass implements methods with incorrect signatures.
class ParentInterface:
def method_one(self, param: int) -> str:
raise NotImplementedError("Method one not implemented")
def method_two(self, value: str) -> int:
raise NotImplementedError("Method two not implemented")
def validate_signature(self, method_name: str, param_type: type, return_type: type) -> None:
method = getattr(self, method_name)
if method.__annotations__ != {param_type.__name__: param_type, 'return': return_type}:
raise TypeError(f"Invalid signature for {method_name}")| @@ -49,21 +49,36 @@ def __init_subclass__(cls, **kwargs): | |||
| # raise Exception(inspect.getmembers(cls, predicate=inspect.isfunction)) | |||
| for defined_method in ( | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting validation logic into separate methods to improve code organization
The validation logic can be simplified by extracting the checks into separate methods while maintaining all functionality. Here's a suggested refactor:
def __init_subclass__(cls, **kwargs):
parent_class = inspect.getmro(cls)[1]
for method_name, _ in inspect.getmembers(cls, predicate=inspect.isfunction):
if method_name.startswith("__"):
continue
cls._validate_method_exists(method_name, parent_class)
cls._validate_method_override(method_name, parent_class)
cls._validate_signatures(method_name, parent_class)
super().__init_subclass__(**kwargs)
@classmethod
def _validate_method_exists(cls, method_name: str, parent_class: type) -> None:
if not hasattr(parent_class, method_name):
raise NotImplementedError(
f"Method {method_name} must be implemented in class '{parent_class.__name__}'"
)
@classmethod
def _validate_method_override(cls, method_name: str, parent_class: type) -> None:
if getattr(parent_class, method_name) is getattr(cls, method_name):
raise NotImplementedError(
f"Subclass '{cls.__name__}' must override the method '{method_name}' from the parent class '{parent_class.__name__}'."
)
@classmethod
def _validate_signatures(cls, method_name: str, parent_class: type) -> None:
parent_params = get_type_hints(getattr(parent_class, method_name))
subclass_params = get_type_hints(getattr(cls, method_name))
if len(parent_params) != len(subclass_params):
raise NotImplementedError(f"Method parameters mismatch:\nExpected: {parent_params.keys()}\nGot: {subclass_params.keys()}")
for parent_sig, subclass_sig in zip(parent_params.items(), subclass_params.items()):
match_signature(parent_sig, subclass_sig)This refactoring:
- Separates validation into clear, single-purpose methods
- Removes nested conditions and walrus operators
- Makes the validation flow linear and easy to follow
- Maintains all validation checks and error messages
| # Check if method is defined in parent class | ||
| if not ( | ||
| hasattr(parent_class, defined_method) | ||
| and (parent_cls_method := getattr(parent_class, defined_method)) |
There was a problem hiding this comment.
suggestion (review_instructions): Assignment expressions (:=) should be used sparingly according to Google style guide
While the walrus operator is allowed, Google's style guide recommends using it sparingly and only when it makes the code significantly more readable. Consider splitting this into two separate lines for better clarity.
Review instructions:
Path patterns: **/*.py, **/*.js, **/*.ex
Instructions:
Check code for Google style guide for python code.
| match="Subclass 'MissingMethodImplementation' must override the method 'method_two' from the parent class 'ParentInterface'.", | ||
| ): | ||
| # A subclass missing a required method | ||
| class MissingMethodImplementation(ParentInterface, InterfaceProtocolCheckMixin): |
There was a problem hiding this comment.
you can move all components needed before test run before the actual tests. this will be your setup and help in the future for easier reading.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @ugly-custard - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def method_two(self, value: str) -> int: | ||
| """Method two Base Parent Interface Class""" | ||
| raise NotImplementedError("Method two not implemented") | ||
|
|
||
|
|
||
| def test_correct_implementation(): | ||
| # A correct subclass implementation | ||
| class CorrectImplementation(ParentInterface, InterfaceProtocolCheckMixin): | ||
| def method_one(self, param: int) -> str: | ||
| return str(param) |
There was a problem hiding this comment.
suggestion (testing): Suggest adding a test case for incorrect parameter types.
It's important to verify that the mixin correctly raises an error when the subclass's method signature doesn't match the parent's, including cases with incorrect parameter types or return types.
Suggested implementation:
def test_correct_implementation():
# A correct subclass implementation
class CorrectImplementation(ParentInterface, InterfaceProtocolCheckMixin):
def method_one(self, param: int) -> str:
return str(param)
def method_two(self, value: str) -> int:
return len(value)
def test_incorrect_parameter_types():
# A subclass with incorrect parameter types
class IncorrectParameterTypes(ParentInterface, InterfaceProtocolCheckMixin):
def method_one(self, param: str) -> str: # param should be int
return param
def method_two(self, value: int) -> int: # value should be str
return value
# Should raise TypeError due to incompatible method signatures
with pytest.raises(TypeError) as exc_info:
IncorrectParameterTypes()
assert "method_one" in str(exc_info.value)
assert "incompatible type" in str(exc_info.value).lower()
You'll need to add the following import at the top of the file if not already present:
import pytest
Related Issue
#33
Changes Done
Updated the InterfaceProtocolCheckMixin to check for missing methods
Wrote tests for it in tests/test_mixins.py
Summary by Sourcery
Tests: