-
Notifications
You must be signed in to change notification settings - Fork 219
APICallWrapper.__get__ must return a bound method #218
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
Conversation
This magic method was previously returning a plain, unbound function. This tricks some introspection code, which expects it to be a bound method. Also add unit test coverage for that module. Bump version to 2.0.0a6.
|
Verified that @jmoldow has signed the CLA. Thanks for the pull request! |
|
|
||
| def test_api_call_decorated_method_must_be_a_cloneable_method(): | ||
|
|
||
| class Cls(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could call this NonCloneable instead of Cls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| def test_api_call_decorated_method_delegates_to_wrapped_method(mock_cloneable, api_call_result): | ||
| args = (1, 2, 'ƒøø', 'bar') | ||
| kwargs = {'bar': 'ƒøø'} | ||
| assert mock_cloneable.api_call_method(*args, **kwargs) == (mock_cloneable, args, kwargs, api_call_result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could take the api_call_method fixture, and call it instead of encoding here the functionality of the method
assert mock_cloneable.api_call_method(*args, **kwargs) == api_call_method(mock_cloneable, *args, **kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to leave this as-is.
If I do what you suggest, then we'll be passing through @api_call on both the LHS and RHS of the equality.
I could expose a fixture for the unbound method, but I'd rather not do that. I think it's fine as-is.
| args = (1, 2, 'ƒøø', 'bar') | ||
| kwargs = {'bar': 'ƒøø'} | ||
| api_call_method = cloneable_subclass_with_api_call_method.api_call_method | ||
| assert api_call_method(mock_cloneable, *args, **kwargs) == (mock_cloneable, args, kwargs, api_call_result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here
This magic method was previously returning a plain, unbound
function. This tricks some introspection code, which expects it
to be a bound method.
Also add unit test coverage for that module.
Bump version to 2.0.0a6.