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

Decorator preserve signature #136

Closed

Conversation

@ankitpatel96
Copy link
Contributor

@ankitpatel96 ankitpatel96 commented Dec 3, 2018

Hi,
We've been using your caching framework for some time now and we've discovered an issue where the decorators @cache_on_arguments (and @cache_multi_on_arguments) don't preserve the signatures of the decorated functions. An example:

In [1]: from dogpile.cache import make_region

In [2]: region = make_region().configure(
   ...:     'dogpile.cache.memory'
   ...: )

In [3]: import inspect

In [4]: def func(a, b, c=True, *args, **kwargs):
   ...:     return None

In [5]: inspect.getargspec(func)
Out[5]: ArgSpec(args=['a', 'b', 'c'], varargs='args', keywords='kwargs', defaults=(True,))

In [6]: cached_func = region.cache_on_arguments()(func)

In [7]: inspect.getargspec(cached_func)
Out[7]: ArgSpec(args=[], varargs='arg', keywords='kw', defaults=None)

To remedy this, I'm submitting a PR that uses the decorator module to preserve the signature of functions decorated by cache_on_arguments.

I've also added tests to check this behavior.

I wasn't sure where to put a new dependency, so I put it in setup.py under install_requires. Let me know if there's a better place for it

Thanks!

@@ -58,6 +58,7 @@ def run_tests(self):
dogpile.cache = dogpile.cache.plugins.mako_cache:MakoPlugin
""",
zip_safe=False,
install_requires=['decorator<5'],
Copy link
Member

@zzzeek zzzeek Dec 3, 2018

would rather remove the <5 here, I wouldn't worry that some major breaking change is coming in decorator 5

@zzzeek
Copy link
Member

@zzzeek zzzeek commented Dec 3, 2018

issue created at #137

@zzzeek
Copy link
Member

@zzzeek zzzeek commented Dec 3, 2018

Dear contributor -

This pull request is being moved to Gerrit, at https://gerrit.sqlalchemy.org/#/c/sqlalchemy/dogpile.cache/+/996, where it may be tested and reviewed more closely. As such, the pull request itself is being marked "closed" or "declined", however your contribution is merely being moved to our central review system. Please register at https://gerrit.sqlalchemy.org#/register/ to send and receive comments regarding this item.

@zzzeek zzzeek closed this Dec 3, 2018
sqlalchemy-bot pushed a commit that referenced this issue Dec 8, 2018
The ``decorator`` module is now used when creating function decorators
within :meth:`.CacheRegion.cache_on_arguments` and
:meth:`.CacheRegion.cache_multi_on_arguments` so that function signatures
are preserved.  Pull request courtesy ankitpatel96.

Additionally adds a small performance enhancement which is to avoid
internally creating a ``@wraps()`` decorator for the creator function on
every get operation, by allowing the arguments to the creator be passed
separately to :meth:`.CacheRegion.get_or_create`.

Co-authored-by: Mike Bayer <mike_mp@zzzcomputing.com>
Fixes: #137
Change-Id: Id1008da596445770cfbf19402d38169aa60678b0
Pull-request: #136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants