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

Alert the user if they are using an unbound class methods to add_view #1498

Merged
merged 5 commits into from
Feb 8, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Raise errors if unbound methods are passed in
  • Loading branch information
sontek committed Dec 27, 2014
commit 4a7029f6b313b65ba94d0726042ea3adbad38e81
17 changes: 17 additions & 0 deletions pyramid/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,3 +244,20 @@ def unquote_bytes_to_wsgi(bytestring):
def is_bound_method(ob):
return inspect.ismethod(ob) and getattr(ob, im_self, None) is not None

def is_unbound_method(fn):
"""
This consistently verifies that the callable is bound to a
class.
"""
is_bound = is_bound_method(fn)

if not is_bound and inspect.isroutine(fn):
spec = inspect.getargspec(fn)
has_self = len(spec.args) > 0 and spec.args[0] == 'self'
Copy link
Member

Choose a reason for hiding this comment

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

Assuming they use the convention of "self" as the first method argument and that they never accidentally use it on something else. I'm all in favor of better feedback so long as it doesn't break someone's valid code. This is probably esoteric enough to not matter but I had to bring it up. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

It's interesting that you brought this up, because when i first read over this code I had the same question, but I let it go because I thought that if someone isn't using self, they are writing very unpythonic code...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, and they will just continue getting the existing poor behavior if they don't use self

Copy link
Member

Choose a reason for hiding this comment

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

We can't try and save the user from themselves every time. I think with the fixes you (@sontek) added we have already helped out 95% of the people that might make a mistake, for the rest we will get a ticket or someone asking for help on IRC.


if PY3 and inspect.isfunction(fn) and has_self: # pragma: no cover
return True
elif inspect.ismethod(fn):
return True

return False
15 changes: 6 additions & 9 deletions pyramid/config/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
url_quote,
WIN,
is_bound_method,
is_unbound_method,
is_nonstr_iter,
im_self,
)
Expand Down Expand Up @@ -419,15 +420,11 @@ def __init__(self, **kw):
self.attr = kw.get('attr')

def __call__(self, view):
# Map the attr directly if the passed in view is method and a
# constructor is defined and must be unbound (for backwards
# compatibility)
if inspect.ismethod(view):
is_bound = getattr(view, im_self, None) is not None

if not is_bound:
self.attr = view.__name__
view = view.im_class
if is_unbound_method(view) and self.attr is None:
raise ConfigurationError((
'Unbound method calls are not supported, please set the class '
'as your `view` and the method as your `attr`'
))

if inspect.isclass(view):
view = self.map_class(view)
Expand Down
46 changes: 46 additions & 0 deletions pyramid/tests/test_compat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import unittest

class TestUnboundMethods(unittest.TestCase):
def test_old_style_bound(self):
from pyramid.compat import is_unbound_method

class OldStyle:
def run(self):
return 'OK'

self.assertFalse(is_unbound_method(OldStyle().run))

def test_new_style_bound(self):
from pyramid.compat import is_unbound_method

class NewStyle(object):
def run(self):
return 'OK'

self.assertFalse(is_unbound_method(NewStyle().run))

def test_old_style_unbound(self):
from pyramid.compat import is_unbound_method

class OldStyle:
def run(self):
return 'OK'

self.assertTrue(is_unbound_method(OldStyle.run))

def test_new_style_unbound(self):
from pyramid.compat import is_unbound_method

class NewStyle(object):
def run(self):
return 'OK'

self.assertTrue(is_unbound_method(NewStyle.run))

def test_normal_func_unbound(self):
from pyramid.compat import is_unbound_method

def func():
return 'OK'

self.assertFalse(is_unbound_method(func))
21 changes: 7 additions & 14 deletions pyramid/tests/test_config/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1669,23 +1669,16 @@ class view2(object):
def test_add_view_class_method_no_attr(self):
from pyramid.renderers import null_renderer
from zope.interface import directlyProvides

class ViewClass(object):
def __init__(self, request):
self.request = request

def run(self):
return 'OK'
from pyramid.exceptions import ConfigurationError

config = self._makeOne(autocommit=True)
config.add_view(view=ViewClass.run, renderer=null_renderer)
class DummyViewClass(object):
def run(self): pass

wrapper = self._getViewCallable(config)
context = DummyContext()
directlyProvides(context, IDummy)
request = self._makeRequest(config)
result = wrapper(context, request)
self.assertEqual(result, 'OK')
def configure_view():
config.add_view(view=DummyViewClass.run, renderer=null_renderer)

self.assertRaises(ConfigurationError, configure_view)

def test_derive_view_function(self):
from pyramid.renderers import null_renderer
Expand Down