-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add support for resolving instance attributes in the cache decorator #1235
Conversation
2a9424f
to
db97f03
Compare
This commit makes the documentation and typing around the utility cache decorator a bit more explicit. It also removes generic kwarg cache option in favour of the explicit timeout argument. The only other option accepted by the cache setter, other than `timeout`, is `version`, which we should avoid until there is an explicit reason to do so to keep the interface explicit.
The `skip_cache` option is currently unusued, and in order to use it, the decorated method would have to explicitly declare `skip_cache` as a kwarg option to control the decorator behavior, which is a pattern we should ideally avoid.
db97f03
to
19296c1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1235 +/- ##
===========================================
- Coverage 95.22% 95.22% -0.01%
===========================================
Files 968 969 +1
Lines 35249 35392 +143
===========================================
+ Hits 33566 33701 +135
- Misses 1683 1691 +8 ☔ View full report in Codecov by Sentry. |
19296c1
to
e26b691
Compare
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.
Couple minor remarks. I see no tests for the error cases (ValueError
, AttributeError
). Would be nice to have those, but it's not critical IMO and I don't want to hold you up, so it's optional.
Thanks @pi-sigma, useful improvements all.
I had those in |
This commit expands the cache decorator's key logic to interpolate instance attribute for methods, in addition to the regular arguments already supported. The typical use-case is an API client, where you will want to scope your method cache keys to instance specific attributes, like a specific endpoint, to avoid cache hits on cross-API calls.
e26b691
to
e883163
Compare
Taiga issue #2524.
This will be followed by a PR to actually use this in the ZGW clients (and will probably also remove a few tests that were really just testing the underlying caching logic, which I've included here in a more isolated form by testing the cache behavior directly).