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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Using getargspec from compat
  • Loading branch information
@Alir3z4
Alir3z4 committed Sep 22, 2016
commit 974b94dfd7a0e7db38967e8863c1a7b59b279591
10 changes: 4 additions & 6 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,12 +60,9 @@ def is_simple_callable(obj):
if not (function or method):
return False

if six.PY2:
args, _, _, defaults = inspect.getargspec(obj)
else:
args, _, _, defaults = inspect.getfullargspec(obj)[:4]
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(args) if function else len(args) - 1
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