-
Notifications
You must be signed in to change notification settings - Fork 47
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
Use qualname to avoid key collisions #257
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is nice, I wonder if we should just switch to using __qualname__
across the board by default. we'd want to do a bigger version bump for that. but let's start with this
@@ -4,7 +4,7 @@ | |||
from ..util import langhelpers | |||
|
|||
|
|||
def function_key_generator(namespace, fn, to_str=str): | |||
def function_key_generator(namespace, fn, to_str=str, use_qual_name=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about also in:
def function_multi_key_generator
def kwarg_function_key_generator
return nested | ||
|
||
|
||
def test_function_key_generator_qualname(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would break up each test case here and have them come in using pytest.mark.parametrize as a decorator on def test_function_key_generator_qualname
. so one function, but the cases come in parameters. each parameter results in one test.
) | ||
|
||
|
||
def test_function_key_generator(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, then also for the multi key and kwarg versions
Hi there! This PR adds the possibility to use a function's full qualified name while building the cache key, an attribute that has been available since python 3.3 so it is safe to be relied upon here.
This PR comes from a bug hunting I did today whose root cause was key collision. We had a structure that looked roughly like this:
When both
active
methods are cached, their keys end up colliding because:but:
In [5]: B.active.__qualname__ Out[5]: 'B.active'
Hope this is useful 🙌