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

LocalProxy: add __wrapped__ attribute to refer to wrapped function #924

Merged
merged 1 commit into from
May 9, 2016

Conversation

immerrr
Copy link
Contributor

@immerrr immerrr commented May 6, 2016

This is an attempt to fix pallets/flask#1680.

__wrapped__ attribute is a somewhat unofficial dunder-attribute used in functools and inspect to refer to an underlying function of a decorator. Technically, LocalProxy can wrap callables, so using __wrapped__ attribute does not seem much of a "workaround".

If "local" is not a callable, __wrapped__ slot is not initialized, and thus the proxy.__wrapped__ request will be forwarded to current object as previously.

@immerrr immerrr force-pushed the add-wrapped-attr-to-localproxy branch from b5b2832 to 526d407 Compare May 6, 2016 09:59
@RonnyPfannschmidt
Copy link
Contributor

looks nice to me, can you please extend it with a unit-test for both simple cases as this is basically a new feature changing the external interface of the locals

@immerrr immerrr force-pushed the add-wrapped-attr-to-localproxy branch from 526d407 to fc14e5b Compare May 6, 2016 10:07
@immerrr
Copy link
Contributor Author

immerrr commented May 6, 2016

@RonnyPfannschmidt done!

@immerrr immerrr force-pushed the add-wrapped-attr-to-localproxy branch from fc14e5b to 259ad70 Compare May 6, 2016 10:08
@immerrr immerrr changed the title LocalProxy: add __wrapped__ attribute to refer to wrapped function (WIP) LocalProxy: add __wrapped__ attribute to refer to wrapped function May 6, 2016
@RonnyPfannschmidt
Copy link
Contributor

looks good to me :)

i'd like another pair of eyes on it to be sure these are the semantics we should add to werkzeug
and what kind of release it needs

locals are very important and while this change looks very useful i'd like to avoid unwarranted disruption

@untitaker
Copy link
Contributor

LGTM, and I'm surprised the fix is so straightforward and that there exists a good reasoning that those are correct semantics instead of a hack. Nice job.

  • Do the tests fail properly without the fix? (use git checkout HEAD^1 werkzeug to revert just the fix).
  • Should this be part of a bugfix release?

@immerrr
Copy link
Contributor Author

immerrr commented May 7, 2016

Do the tests fail properly without the fix?

Yes,

=================================== FAILURES ===================================
______________________ test_local_proxy_wrapped_attribute ______________________

    def test_local_proxy_wrapped_attribute():
        class SomeClassWithWrapped(object):
            __wrapped__ = 'wrapped'

        def lookup_func():
            return 42

        partial_lookup_func = partial(lookup_func)

        proxy = local.LocalProxy(lookup_func)
>       assert proxy.__wrapped__ is lookup_func

tests/test_local.py:197: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = 42, name = '__wrapped__'

    def __getattr__(self, name):
        if name == '__members__':
            return dir(self._get_current_object())
>       return getattr(self._get_current_object(), name)
E       AttributeError: 'int' object has no attribute '__wrapped__'

werkzeug/local.py:343: AttributeError
=============== 1 failed, 429 passed, 30 skipped in 1.35 seconds ===============

@untitaker untitaker merged commit 8a84b62 into pallets:master May 9, 2016
@untitaker
Copy link
Contributor

Excellent, thank you!

@immerrr immerrr deleted the add-wrapped-attr-to-localproxy branch May 9, 2016 20:16
@xrmx
Copy link

xrmx commented Jul 31, 2016

@untitaker Any chance this can be backported to 0.11 please?

@untitaker
Copy link
Contributor

Sorry, but I don't consider this to be a bugfix as it originally was never
intended to work that way.

On Sun, Jul 31, 2016 at 07:03:51AM -0700, Riccardo Magliocchetti wrote:

@untitaker Any chance this can be backported to 0.11 please?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#924 (comment)

rtweeks pushed a commit to rtweeks/werkzeug that referenced this pull request Oct 18, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flask isn't doctestable
4 participants