Raise error if method is missing in either parent or subclasse; add tests for mixin #95
Conversation
Reviewer's Guide by SourceryThis PR enhances the InterfaceProtocolCheckMixin by adding stricter method validation between parent and subclasses. The implementation now checks for missing methods in both parent and subclass, and ensures proper method overriding. The changes are accompanied by comprehensive tests that verify the new validation behavior. Updated class diagram for InterfaceProtocolCheckMixinclassDiagram
class InterfaceProtocolCheckMixin {
+__init_subclass__(cls, **kwargs)
}
class ParentInterface {
+method_one(param: int) str
+method_two(value: str) int
}
class CorrectImplementation {
+method_one(param: int) str
+method_two(value: str) int
}
class MissingMethodImplementation {
+method_one(param: int) str
}
class ExtendedImplementation {
+method_one(param: int) str
+method_two(value: str) int
+method_three(extra: float) float
}
ParentInterface <|-- CorrectImplementation
ParentInterface <|-- MissingMethodImplementation
ParentInterface <|-- ExtendedImplementation
InterfaceProtocolCheckMixin <|-- CorrectImplementation
InterfaceProtocolCheckMixin <|-- MissingMethodImplementation
InterfaceProtocolCheckMixin <|-- ExtendedImplementation
note for InterfaceProtocolCheckMixin "Checks for missing methods in parent and subclass"
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.
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
Enhance the InterfaceProtocolCheckMixin to ensure that methods are correctly implemented in both parent and subclass, raising errors if methods are missing. Add comprehensive tests to validate the mixin's behavior in various scenarios.
Enhancements:
Tests: