Fixes #4506: Support Python 3 annotations on fields callables #4508
Conversation
| args, _, _, defaults = inspect.getargspec(obj) | ||
| else: | ||
| args, _, _, defaults = inspect.getfullargspec(obj)[:4] | ||
|
|
There was a problem hiding this comment.
Great stuff. Let's move this into a getargspec() function in compat.py, then we'll be good to go.
There was a problem hiding this comment.
What would the work overhead to use Signature instead of getfullargspec ?
I'm asking because getfullargspec is deprecated from 3.5 so since we're working on that part, might be good to jump directly to the Signature.
There was a problem hiding this comment.
Note that I don't want to bother if it's too much work.
There was a problem hiding this comment.
@tomchristie Yeah, moving to compat makes more sense. Wasn't familiar with the code much. Will do that now.
@xordoquy that makes more sense, yeah. I agree Signature would be better, I just implemented it with the method with inspect.Signature and I'm going to push it.
There was a problem hiding this comment.
@tomchristie - I don't think a compat.getargspec makes sense here, as signature and getargspec behave differently and aren't directly interchangeable. Primarily, signature will not return self for bound methods where as getargspec will. I'll push a test case to demonstrate.
There was a problem hiding this comment.
@rpkilby compat.getargspec only returns the method parameters and defaults and that's all fields.is_simple_callable cares about and nothing else.
If other parts of the application requires something more than that from compat.getargspec then modifications can be applied, otherwise the whole backward compatibility should be taken care of in fields.is_simple_callable itself, which that was what I did in the first place and @tomchristie suggested the change, In my opinion it keeps the code clean and straight.
|
Just pushed the new changes. |
rest_framework/compat.py
Outdated
|
|
||
|
|
||
| def getargspec(obj): # type: tuple | ||
| if not hasattr(inspect, 'signature'): |
There was a problem hiding this comment.
An inline comment referring to which python versions each branch applies to would make sense.
There was a problem hiding this comment.
Also the # type: tuple seems redundant - if we're going to describe the return type could we instead either do so more fully or not at all.
There was a problem hiding this comment.
Return type here has been mentioned as an backward compatible for python mypy for python 2.
I'll remove remove it now an until there's a decision made for such thing.
There was a problem hiding this comment.
@tomchristie I've applied the changes you've suggested.
|
Lemme know if you have more suggestions on this so I can apply. |
|
Looks good. Will make a final review in due course. |
|
Closed via #4510. Incoming in 3.5 |
4 participants
Fixes #4506