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

workaround for bug in functools.update_wrapper for bound methods. #224

Closed
wants to merge 2 commits into from

Conversation

wickman
Copy link

@wickman wickman commented Sep 23, 2011

arguably a fix for #223 though there are other places where functools.update_wrapper is called. this is sufficient for my needs.

@wickman
Copy link
Author

wickman commented Sep 23, 2011

updated. i'd like to write a regression test but it seems to depend upon a vague external library ('tools') -- any pointers?

@defnull
Copy link
Member

defnull commented Sep 23, 2011

Your fix silently ignores class or instance methods. Both are MethodType and not FunctionType.

E.g. someone might want to do this::

controller = MyController()
bottle.route('/', callback=controller.some_method)

This should work as expected and not skip update_wrapper().

I'd prefer a hasattr(module) if that is the only case that triggers the bug.

@wickman
Copy link
Author

wickman commented Sep 23, 2011

I agree it should work, but it doesn't:
https://gist.github.com/1238253

instancemethods have neither __module__ nor __name__ and their __doc__ is immutable.

@defnull
Copy link
Member

defnull commented Nov 6, 2011

Do you know in which versions of python this bug appears? I tested it with 2.6 and it just worked. I am currently re-building my testing environment.

@defnull
Copy link
Member

defnull commented Nov 6, 2011

This code:

class A(object):
  @staticmethod
  def static(): pass

  @classmethod
  def cls(cls): pass

  def method(self): pass

from functools import update_wrapper

def w(): pass

update_wrapper(w, A().method)
update_wrapper(w, A().cls)
update_wrapper(w, A().static)

update_wrapper(w, A.method)
update_wrapper(w, A.cls)
update_wrapper(w, A.static)

works in all 5 supported Python versions: 2.5 2.6 2.7 3.1 3.2

@wickman
Copy link
Author

wickman commented Nov 6, 2011

That's because w is not an instancemethod. A simple way to test the functools issue on 2.6.x:

class A(object):
  def A_instance_method(self):
    pass

class B(object):
  @staticmethod
  def static():
    pass

from functools import update_wrapper
update_wrapper(A().A_instance_method, B.static)

It's legitimate to argue that you should never want to wrap instancemethods, but the gist I linked to does this and it actually works just fine if I monkeypatch functools.update_wrapper to something safe for immutable attributes.

defnull added a commit that referenced this pull request Nov 22, 2011
defnull added a commit that referenced this pull request Nov 22, 2011
@defnull defnull closed this Nov 22, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants