Improve GroupingLatestValueCache
#433
Conversation
Signed-off-by: Leandro Lucarella <[email protected]>
The module documentation of `GroupingLatestValueCache` wasn't updated to match the new `Mapping` interface. Signed-off-by: Leandro Lucarella <[email protected]>
The module documentation is not being included in the rendered docs. Normally this is done by including it in the User Guide, but since this is an experimental module, we don't want to include it yet, so it is easier to just move the docs to the class so they are rendered in the API documentation. Signed-off-by: Leandro Lucarella <[email protected]>
This follows the `dict` interface more closely, so it should be more familiar to users. Signed-off-by: Leandro Lucarella <[email protected]>
github-actions
bot
added
part:tests
llucax
added
the
cmd:skip-release-notes
|
Not that I planned to do this, it just happened as I was adapting the code to do make receivers context managers... |
There was a problem hiding this comment.
Pull Request Overview
This PR refines the GroupingLatestValueCache by updating its documentation, aligning its interface more closely with MutableMapping, and adding integration tests for new mapping methods.
- Updated module and class docstrings for clarity and completeness
- Implemented
__getitem__,__delitem__,pop,popitem, andclearto mirrorMutableMapping - Expanded integration tests to cover indexing, deletion, popping, iteration, and equality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_grouping_latest_value_cache_integration.py | Added tests for __getitem__, __delitem__, pop, popitem, iteration, and equality |
| src/frequenz/channels/experimental/_grouping_latest_value_cache.py | Revised module docstring, updated imports and overloads, implemented removal methods, refined __eq__/__ne__ |
Comments suppressed due to low confidence (2)
src/frequenz/channels/experimental/_grouping_latest_value_cache.py:4
- [nitpick] The module docstring is very brief; consider adding a summary and a blank line followed by a longer description of the module’s purpose and contents per PEP 257.
"""The GroupingLatestValueCache caches the latest values in a receiver grouped by key."""
src/frequenz/channels/experimental/_grouping_latest_value_cache.py:33
- This class implements mutating methods like
pop,popitem,__delitem__, andclear, but only inherits fromMapping. Consider inheriting fromMutableMappingto accurately reflect the supported interface.
class GroupingLatestValueCache(Mapping[HashableT, ValueT_co]):
We can offer all item-clearing methods from `MutableMapping`, we just don't want to allow users to update values. Signed-off-by: Leandro Lucarella <[email protected]>
All other methods are just forwarding operations to the underlying `dict`, so there is no point in handling `KeyError`s manually in this method. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Since the comparison only compares the underlying `dict` and doesn't have into account the receiver, but just the current cache state, we can safely allow comparing to other `Mapping`s too. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Typing symbols are very fundamental and can make the code more readable to use them directly. Signed-off-by: Leandro Lucarella <[email protected]>
| def pop( | ||
| self, key: HashableT, /, default: DefaultT | _NotSpecified = _NotSpecified() | ||
| ) -> ValueT_co | DefaultT | None: |
There was a problem hiding this comment.
| def pop( | |
| self, key: HashableT, /, default: DefaultT | _NotSpecified = _NotSpecified() | |
| ) -> ValueT_co | DefaultT | None: | |
| def pop( | |
| self, key: HashableT, /, default: DefaultT | None = None | |
| ) -> ValueT_co | DefaultT | None: |
Then you can use return self._latest_value_by_key.pop(key, default) as the only body, because that's the exact signature of dict.pop(), and no need for special cases.
And if a user wants to have None in ValueT_co, they can make their own sentinel that will be DefaultT. So we don't need to make a sentinel to be able to satisfy MutableMapping.
There was a problem hiding this comment.
No, the problem is pop() raises when the default is not specified, it doesn't return None (is not symmetric with get()), so we need to be able to tell between not passing the argument and passing None.
There was a problem hiding this comment.
oh damn, that's such a bad API.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Mmm, failing in Python 3.12, will have to have a look. |
Importing those symbols from `typing` is deprecated. Signed-off-by: Leandro Lucarella <[email protected]>
Strange that it only fails in 3.12. Updated with a new commit with a fix. |
2 participants
Fixes some documentation issues and brings the interface more close to
MutableMapping.