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
Next Next commit
Fixes #4506: Support Python 3 annotations on fields callables
  • Loading branch information
@Alir3z4
Alir3z4 committed Sep 22, 2016
commit 6013bcb42506b9822c43dfe8e3dff5d2762c5185
6 changes: 5 additions & 1 deletion rest_framework/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ def is_simple_callable(obj):
if not (function or method):
return False

args, _, _, defaults = inspect.getargspec(obj)
if six.PY2:
args, _, _, defaults = inspect.getargspec(obj)
else:
args, _, _, defaults = inspect.getfullargspec(obj)[:4]

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_defaults = len(defaults) if defaults else 0
return len_args <= len_defaults
Expand Down