Skip to content

Fixes #4506: Support Python 3 annotations on fields callables #4508

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.

Sign up for GitHub

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

Closed
Alir3z4 wants to merge 5 commits into from
Closed

Fixes #4506: Support Python 3 annotations on fields callables #4508

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6013bcb
Fixes #4506: Support Python 3 annotations on fields callables
Alir3z4 Sep 22, 2016
fe417aa
Add getargspec to compact
Alir3z4 Sep 22, 2016
974b94d
Using getargspec from compat
Alir3z4 Sep 22, 2016
610e8ca
Better to check of existence of `signature`
Alir3z4 Sep 22, 2016
14c22bc
Coment t o show whihc python version is statement for
Alir3z4 Sep 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions rest_framework/compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,16 @@ def value_from_object(field, obj):
return field.value_from_object(obj)


def getargspec(obj): # type: tuple
if not hasattr(inspect, 'signature'):
Copy link
Contributor

@lovelydinosaur lovelydinosaur Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An inline comment referring to which python versions each branch applies to would make sense.

Copy link
Contributor

@lovelydinosaur lovelydinosaur Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Alir3z4 Alir3z4 Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Alir3z4 Alir3z4 Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomchristie I've applied the changes you've suggested.

parameters, _, _, defaults = inspect.getargspec(obj)
else:
signature = inspect.signature(obj)
parameters = signature.parameters
defaults = [i for i in parameters if i.default != inspect._empty]

return parameters, defaults

# contrib.postgres only supported from 1.8 onwards.
try:
from django.contrib.postgres import fields as postgres_fields
Expand Down
8 changes: 5 additions & 3 deletions rest_framework/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@

from rest_framework import ISO_8601
from rest_framework.compat import (
get_remote_field, unicode_repr, unicode_to_repr, value_from_object
get_remote_field, getargspec, unicode_repr, unicode_to_repr,
value_from_object
)
from rest_framework.exceptions import ValidationError
from rest_framework.settings import api_settings
Expand All @@ -59,8 +60,9 @@ def is_simple_callable(obj):
if not (function or method):
return False

args, _, _, defaults = inspect.getargspec(obj)
len_args = len(args) if function else len(args) - 1
parameters, defaults = getargspec(obj)

Copy link
Contributor

@lovelydinosaur lovelydinosaur Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff. Let's move this into a getargspec() function in compat.py, then we'll be good to go.

Copy link
Contributor

@xordoquy xordoquy Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@xordoquy xordoquy Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I don't want to bother if it's too much work.

Copy link
Author

@Alir3z4 Alir3z4 Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@rpkilby rpkilby Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here in #4505.

Copy link
Contributor

@rpkilby rpkilby Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

@Alir3z4 Alir3z4 Sep 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

len_args = len(parameters) if function else len(parameters) - 1
len_defaults = len(defaults) if defaults else 0
return len_args <= len_defaults

Expand Down