Skip to content
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

Jump to bottom

Fix is_simple_callable with variable args, kwargs #4622

Merged
merged 2 commits into from Oct 25, 2016
Merged

Fix is_simple_callable with variable args, kwargs #4622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91d6fb1
Add regression test for #4602
Oct 24, 2016
8952096
Check args and kwargs in is_simple_callable
Oct 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions rest_framework/fields.py
View file
Open in desktop
Expand Up @@ -54,12 +54,17 @@ def is_simple_callable(obj):
"""
True if the object is a callable that takes no arguments.
"""
if not callable(obj):
if not (inspect.isfunction(obj) or inspect.ismethod(obj)):
return False

sig = inspect.signature(obj)
params = sig.parameters.values()
return all(param.default != param.empty for param in params)
return all(
param.kind == param.VAR_POSITIONAL or
param.kind == param.VAR_KEYWORD or
param.default != param.empty
for param in params
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rpkilby: Can you just explain this call to me, just for my sanity? 🙂

I'm expecting any(iterable) — it looks like bool or bool or generator, so why isn't it this:

>>> a = True
>>> b = True
>>> def c():
...     yield True
...     yield False
>>> all(a or b or c())
TypeError: 'bool' object is not iterable

Sorry for being slow. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. 1 second after Comment it becomes clear. ... — even seeing it though, I still struggle to read it right...

Copy link
Member Author

Choose a reason for hiding this comment

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

@carltongibson - it probably wouldn't have hurt to have added a second set of parens wrapping the conditions

return all((
        param.kind == param.VAR_POSITIONAL or
        param.kind == param.VAR_KEYWORD or
        param.default != param.empty
    ) for param in params
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's kind to of where I came to. (That vs a nested any)

It's fine.


else:
def is_simple_callable(obj):
Expand Down
23 changes: 23 additions & 0 deletions tests/test_fields.py
View file
Open in desktop
Expand Up @@ -37,6 +37,9 @@ def valid(self):
def valid_kwargs(self, param='value'):
pass

def valid_vargs_kwargs(self, *args, **kwargs):
pass

def invalid(self, param):
pass

Expand All @@ -45,11 +48,13 @@ def invalid(self, param):
# unbound methods
assert not is_simple_callable(Foo.valid)
assert not is_simple_callable(Foo.valid_kwargs)
assert not is_simple_callable(Foo.valid_vargs_kwargs)
assert not is_simple_callable(Foo.invalid)

# bound methods
assert is_simple_callable(Foo().valid)
assert is_simple_callable(Foo().valid_kwargs)
assert is_simple_callable(Foo().valid_vargs_kwargs)
assert not is_simple_callable(Foo().invalid)

def test_function(self):
Expand All @@ -59,13 +64,31 @@ def simple():
def valid(param='value', param2='value'):
pass

def valid_vargs_kwargs(*args, **kwargs):
pass

def invalid(param, param2='value'):
pass

assert is_simple_callable(simple)
assert is_simple_callable(valid)
assert is_simple_callable(valid_vargs_kwargs)
assert not is_simple_callable(invalid)

def test_4602_regression(self):
from django.db import models

class ChoiceModel(models.Model):
choice_field = models.CharField(
max_length=1, default='a',
choices=(('a', 'A'), ('b', 'B')),
)

class Meta:
app_label = 'tests'

assert is_simple_callable(ChoiceModel().get_choice_field_display)

@unittest.skipUnless(typings, 'requires python 3.5')
def test_type_annotation(self):
# The annotation will otherwise raise a syntax error in python < 3.5
Expand Down