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

feat: on-get callback #38

Merged
merged 3 commits into from
Nov 13, 2023
Merged

feat: on-get callback #38

merged 3 commits into from
Nov 13, 2023

Conversation

uncle-lv
Copy link
Contributor

@uncle-lv uncle-lv commented Nov 9, 2023

Usage

set on_get callback.

cache = Cache(on_get=on_get)

on_get is a callable object.

Callable[[key: Hashable, value: Any, existed: bool],  None]

key is the key of cache entry has been got.

value is the real value when the key has existed. Otherwise, it's the default value.

exsited indicates whether the key has existed.

@coveralls
Copy link

coveralls commented Nov 9, 2023

Coverage Status

coverage: 100.0%. remained the same
when pulling d3de821 on uncle-lv:on-get-callback
into 346c6b4 on dgilland:master.

Comment on lines 77 to 78
on_get: Callback which will be executed when a cache entry is got.
on_delete: Callback which will be executed when a cache entry is removed.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to include the excepted method signatures for on_get and on_delete in terms of what the positional arguments are since just the types don't give enough context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any example?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
on_get: Callback which will be executed when a cache entry is got.
on_delete: Callback which will be executed when a cache entry is removed.
on_get: Callback that will be executed when a cache entry is retrieved. It is called with arguments ``(key, value, exists)`` where `key` is the cache key, `value` is the value retrieved (could be the default), and `exists` is whether the cache key exists or not.
on_delete: Callback that will be executed when a cache entry is removed. It is called with arguments ``(key, value, cause)`` where `key` is the cache key, `value` is the cached value at the time of deletion, and `cause` is the reason the key was removed (see :class:`RemovalCause` for enumerated causes).

@@ -74,6 +74,7 @@ class Cache:
default: Default value or function to use in :meth:`get` when key is not found. If callable,
it will be passed a single argument, ``key``, and its return value will be set for that
cache key.
on_get: Callback which will be executed when a cache entry is got.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of is got, maybe is retrieved.

Comment on lines 269 to 271
if self.on_get:
self.on_get(key, value, True)
except KeyError:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an edge-case here where if self.on_get() throws a KeyError, then we end up calling it twice. Maybe we should defer calling self.on_get until after the try/except block. That way we'd only have a single place where it's called and if it happens to raise a KeyError, we won't mistakenly catch it.

Comment on lines 21 to 26
F = t.TypeVar("F", bound=t.Callable[..., t.Any])
T_DECORATOR = t.Callable[[F], F]
T_TTL = t.Union[int, float]
T_FILTER = t.Union[str, t.List[t.Hashable], t.Pattern, t.Callable]

UNSET = object()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should stay at the top right below the imports.

T_DECORATOR = t.Callable[[F], F]
T_TTL = t.Union[int, float]
T_FILTER = t.Union[str, t.List[t.Hashable], t.Pattern, t.Callable]
ON_GET_CALLBACK = t.Optional[t.Callable[[t.Hashable, t.Any, bool], None]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix these with T_*

Comment on lines 46 to 52
"""
Callback that will be executed when a cache entry is retrieved.

It is called with arguments ``(key, value, exists)`` where `key` is the cache key,
`value` is the value retrieved (could be the default),
and `exists` is whether the cache key exists or not.
"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert these to #: * prefixed comments above the variable declaration like:

#: Callback that will be executed when a cache entry is retrieved. It is called with arguments
#: ``(key, value, exists)`` where `key` is the cache key, `value` is the value retrieved (could be
#: the default), and `exists` is whether the cache key exists or not.
T_ON_DELETE_CALLBACK = ...

Comment on lines 94 to 95
on_get: Callback which will be executed when a cache entry is retrieved.
on_delete: Callback which will be executed when a cache entry is removed.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking of putting the variable descriptions here was so that it would be prominent when viewing the class documentation. Does any of the details from the constant docstrings show up here in the generated docs?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could add a class reference so that there's at least a link to the variable docs using something like:

See :class:`T_ON_DELETE_CALLBACK`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking of putting the variable descriptions here was so that it would be prominent when viewing the class documentation. Does any of the details from the constant docstrings show up here in the generated docs?

Yes, it works.

Comment on lines +50 to +62
#: Callback that will be executed when a cache entry is retrieved.

#: It is called with arguments ``(key, value, exists)`` where `key` is the cache key,
#: `value` is the value retrieved (could be the default),
#: and `exists` is whether the cache key exists or not.
T_ON_GET_CALLBACK = t.Optional[t.Callable[[t.Hashable, t.Any, bool], None]]

#: Callback that will be executed when a cache entry is removed.

#: It is called with arguments ``(key, value, cause)`` where `key` is the cache key,
#: `value` is the cached value at the time of deletion,
#: and `cause` is the reason the key was removed (see :class:`RemovalCause` for enumerated causes).
T_ON_DELETE_CALLBACK = t.Optional[t.Callable[[t.Hashable, t.Any, RemovalCause], None]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Constants should all be at the top after the imports. Class definitions next and then function definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T_ON_DELETE_CALLBACK must be after RemovalCause because it depends on RemovalCause.

@dgilland dgilland merged commit bcabcdb into dgilland:master Nov 13, 2023
14 checks passed
@dgilland dgilland mentioned this pull request Nov 13, 2023
@uncle-lv uncle-lv deleted the on-get-callback branch November 14, 2023 01:14
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.

3 participants