Extend patch decorator to add methods to class types#11932
Conversation
giograno
added
the
semver: patch
thrau
left a comment
thrau
left a comment
There was a problem hiding this comment.
nice new feature! just a couple of comments regarding code organization and implementation details
| if inspect.isclass(target): | ||
| # Special case: when a class type is passed, we bound the function to the class type patching __getattr__ | ||
| return Patch._extend_class(target, fn) |
There was a problem hiding this comment.
i think having this check here doesn't make a ton of sense, since this is explicitly to patch functions. i think this check should be in patch
| return self | ||
|
|
||
| @staticmethod | ||
| def _extend_class(target: Callable, fn: Callable): |
There was a problem hiding this comment.
i would put this on the same footing as function
| def _extend_class(target: Callable, fn: Callable): | |
| def extend_class(target: Callable, fn: Callable): |
| try: | ||
| self.old = getattr(self.obj, name) | ||
| if self.old and name == "__getattr__": | ||
| raise Exception("You can't patch class types implementing __getattr__") | ||
| except AttributeError as e: | ||
| # When we want to add a function to a class type, we want to intercept this exception and check if the | ||
| # name is `__getattr__`. | ||
| if name != "__getattr__": | ||
| raise e | ||
| self.old = None |
There was a problem hiding this comment.
i think this check should be in apply so we don't cause exceptions while instantiating the object
thrau
left a comment
thrau
left a comment
There was a problem hiding this comment.
this is much cleaner, thanks! just one minor nit that i missed, please just amend and merge :-)
| return self | ||
|
|
||
| @staticmethod | ||
| def extend_class(target: Callable, fn: Callable): |
There was a problem hiding this comment.
nit: sorry i missed this, but the type hint should by Type, right?
2 participants
Motivation
Our
patchdecorator easily allows us to patch a class method and extend its behavior.However, it does not support the case where a given type does not implement a function and we want to add it.
Patch now would support the following use case:
If the class already implements a method with the given name, the patch won't have any effect.
Like our current implementation, a patch can be easily undone:
Changes
patchto support the use case of adding a new function to a class type;