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

sontek · 2014-12-26T07:48:50Z

This prevents the following from giving an unrecognizable error:

from pyramid.config import Configurator
from pyramid.response import Response
from waitress import serve

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

    def hello_world(self):
        return Response('Hello world!')

def create_app():
    config = Configurator()
    config.add_route('hello_world', '/')
    config.add_view(View.hello_world, route_name='hello_world')

    app = config.make_wsgi_app()
    return app

if __name__ == '__main__':
    app = create_app()
    serve(app)

Before this PR you would get the exception:

Traceback (most recent call last):
  File "app.py", line 21, in <module>
    app = create_app()
  File "app.py", line 17, in create_app
    app = config.make_wsgi_app()
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/__init__.py", line 956, in make_wsgi_app
    self.commit()
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/__init__.py", line 625, in commit
    self.action_state.execute_actions(introspector=self.introspector)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/__init__.py", line 1084, in execute_actions
    tb)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/__init__.py", line 1076, in execute_actions
    callable(*args, **kw)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 1236, in register
    derived_view = deriver(view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 159, in __call__
    view)))))))))
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 103, in inner
    wrapper_view = wrapper(self, view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 171, in mapped_view
    mapped_view = mapper(**self.kw)(view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 424, in __call__
    view = self.map_nonclass(view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 462, in map_nonclass
    mapped_view.__text__ = object_description(view)
pyramid.exceptions.ConfigurationExecutionError: <type 'exceptions.AttributeError'>: 'instancemethod' object has no attribute '__text__'
  in:
  Line 15 of file app.py:
    config.add_view(View.hello_world, route_name='hello_world')

tseaver · 2014-12-26T11:53:30Z

Can you fix the example above such that it shows a "normal" signature for the hello_world method (taking self as well as request)? Also, fix the one in the test?

tseaver · 2014-12-26T12:00:21Z

Other than that, LGTM.

sontek · 2014-12-26T12:05:34Z

@tseaver The request would be passed in to the constructor of the class (same as when using attr), so the view func itself wouldn't get one. Are you thinking we would detect if they provided a __init__ that accepts request and if it doesn't pass request to the function?

tseaver · 2014-12-26T12:12:13Z

Ah, you mean in the test? Yes, I'd neglected to think about the "passed to __init__" part. Looks like the motivating example above no longer shows request as the sole argument to hello_world.

sontek · 2014-12-26T12:19:14Z

@tseaver Yeah, the example in my comment was an error, I fixed that. But the test should be accurate

mmerickel · 2014-12-27T02:54:55Z

This worries me. In Python3 unbound methods do not exist, so when you do View.hello_world you are actually just getting a function... not a method. How does this PR handle that concept?

sontek · 2014-12-27T03:48:21Z

@mmerickel Thats the inconsistent API part of Pyramid I was talking about. If you use attr is passes request to the constructor, otherwise it passes it to the function. I didn't realize but this actually does break on py3x.

I've updated the test to blow up on py3x but the only way I can think to fix it will only work in Python 3.3+, Python 3.1 and 3.2 wont work. So this might be something we can't do since Python3x doesn't have the feature that Python2x does

sontek · 2014-12-27T06:59:09Z

Interesting, I have no idea why coverage dropped there. Those lines are all marked #pragma: no cover. I'll look into it.

sontek · 2014-12-27T07:26:46Z

@tseaver @mmerickel Rather than trying to support this (since getting the class of a class function is pretty much impossible in py3), I decided to just raise a cleaner exception so that it is obvious that you are using the API wrong:

Traceback (most recent call last):
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/__init__.py", line 1076, in execute_actions
    callable(*args, **kw)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 1243, in register
    derived_view = deriver(view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 160, in __call__
    view)))))))))
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 104, in inner
    wrapper_view = wrapper(self, view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 172, in mapped_view
    mapped_view = mapper(**self.kw)(view)
  File "/home/sontek/venvs/pyramid/src/pyramid/pyramid/config/views.py", line 424, in __call__
    'Unbound method calls are not supported, please set the class '
pyramid.exceptions.ConfigurationError: Unbound method calls are not supported, please set the class as your `view` and the method as your `attr`

mmerickel · 2015-02-06T05:04:17Z

pyramid/compat.py

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


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?

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

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

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.

digitalresistor · 2015-02-06T05:09:37Z

👍

mmerickel · 2015-02-06T06:31:15Z

@sontek can you merge master into this so I can hit the little green button?

…_class_method_directly Conflicts: pyramid/compat.py

sontek · 2015-02-07T20:36:27Z

@mmerickel Its merged with master now

mmerickel · 2015-02-08T03:19:36Z

Not a fan of you slowly pep-8ing the code in these PRs, but thanks anyway!

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

digitalresistor · 2015-02-08T04:06:00Z

All of the extra whitespace changes are noise and make it difficult to see the meat in the changeset.

Add support for passing unbound class methods to add_view

bc26deb

Raise errors if unbound methods are passed in

4a7029f

sontek added 2 commits December 26, 2014 23:04

Don't need im_self

6d46761

Clean up compat tests

03a0d79

sontek changed the title ~~Add support for passing unbound class methods to add_view~~ Alert the user if they are using an unbound class methods to add_view Dec 27, 2014

mmerickel reviewed Feb 6, 2015
View reviewed changes

mmerickel added this to the 1.6 milestone Feb 6, 2015

Merge branch 'master' of https://github.com/Pylons/pyramid into allow…

ce7c06a

…_class_method_directly Conflicts: pyramid/compat.py

mmerickel added a commit that referenced this pull request Feb 8, 2015

Merge pull request #1498 from sontek/allow_class_method_directly

4517ec5

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

mmerickel merged commit 4517ec5 into Pylons:master Feb 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sontek commented Dec 26, 2014

tseaver commented Dec 26, 2014

tseaver commented Dec 26, 2014

sontek commented Dec 26, 2014

tseaver commented Dec 26, 2014

sontek commented Dec 26, 2014

mmerickel commented Dec 27, 2014

sontek commented Dec 27, 2014

sontek commented Dec 27, 2014

sontek commented Dec 27, 2014

mmerickel Feb 6, 2015

digitalresistor Feb 6, 2015

sontek Feb 6, 2015

digitalresistor Feb 6, 2015

digitalresistor commented Feb 6, 2015

mmerickel commented Feb 6, 2015

sontek commented Feb 7, 2015

mmerickel commented Feb 8, 2015

digitalresistor commented Feb 8, 2015

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

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

Conversation

sontek commented Dec 26, 2014

tseaver commented Dec 26, 2014

tseaver commented Dec 26, 2014

sontek commented Dec 26, 2014

tseaver commented Dec 26, 2014

sontek commented Dec 26, 2014

mmerickel commented Dec 27, 2014

sontek commented Dec 27, 2014

sontek commented Dec 27, 2014

sontek commented Dec 27, 2014

mmerickel Feb 6, 2015

Choose a reason for hiding this comment

digitalresistor Feb 6, 2015

Choose a reason for hiding this comment

sontek Feb 6, 2015

Choose a reason for hiding this comment

digitalresistor Feb 6, 2015

Choose a reason for hiding this comment

digitalresistor commented Feb 6, 2015

mmerickel commented Feb 6, 2015

sontek commented Feb 7, 2015

mmerickel commented Feb 8, 2015

digitalresistor commented Feb 8, 2015

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

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