refactor: switch input parameters from dict to Mapping #103
Conversation
Switch all input parameters from `dict[str, Any]` to `Mapping[str, Any]` while keeping return types as `dict[str, Any]`. This follows the Liskov Substitution Principle - accept more general types as inputs, return more specific types as outputs. This change improves the API's flexibility by accepting any mapping-like object (dict, OrderedDict, custom mappings, etc.) as input while maintaining backward compatibility. All existing code continues to work since dict is a subclass of Mapping. Changes: - Updated AsyncKeyValueProtocol with Mapping for put() and put_many() parameters - Updated ManagedEntry.value field to accept Mapping[str, Any] - Updated BaseStore and all wrapper implementations - Updated adapters (RaiseOnMissingAdapter) - Added dict() conversions where needed (encryption, compression, limit_size wrappers) Note: Sync library will need to be regenerated from the async version. Fixes #94 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: William Easton <strawgate@users.noreply.github.com>
📝 WalkthroughWalkthroughPublic APIs and internal signatures across async/sync protocols, stores, adapters, wrappers, tests, and shared utilities were changed to accept Mapping[str, Any] (and Sequence[Mapping[str, Any]] for batch ops) instead of concrete dict[str, Any]; Mapping imports were added and a few places convert mappings to dict(...) for processing/serialization. Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (7)
🧰 Additional context used🧬 Code graph analysis (6)key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.py (3)
key-value/key-value-aio/src/key_value/aio/wrappers/logging/wrapper.py (4)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/logging/wrapper.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
🔇 Additional comments (26)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@claude you lint and codegen please |
|
Claude finished @strawgate's task —— View job Running lint and codegenTasks
SummaryI attempted to run the requested linting and code generation tasks, but I don't have permission to execute the necessary commands:
What you can doYou have a few options:
According to the Makefile, the --- | |
|
@claude you have access now try again |
|
Claude finished @strawgate's task —— View job Running lint and codegenTasks
SummarySuccessfully ran linting and code generation: Linting: All checks passed with no changes needed
Code Generation: Successfully regenerated sync library
The sync library is now up to date with the async library changes from this PR. |
Regenerated the sync library from async sources to reflect the changes from PR #103 which switched input parameters from dict to Mapping. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
@claude looks like type checking is failing on the air project maybe you didn't change the mock stores in the tests? Please review |
|
Claude finished @strawgate's task —— View job Type checking issues resolvedSummaryFixed all type checking failures related to the Mapping refactor. The issues were caused by missing type conversions between Issues Found and Fixed
Verification
The CI should now pass. The key insight is that while we accept |
- Fix BaseStore return types by converting Mapping to dict in get/ttl methods - Update LoggingWrapper internal methods to accept Mapping types - Fix test mock store to use Mapping instead of dict for protocol compatibility - Fix ManagedEntry.to_json() to convert Mapping to dict - Regenerate sync library with all fixes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: William Easton <strawgate@users.noreply.github.com>
|
@coderabbitai please review again |
|
@strawgate, I'll review the changes in this PR for you now. ✅ Actions performedReview triggered.
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the key-value library to accept Mapping[str, Any] instead of dict[str, Any] for input parameters while maintaining dict[str, Any] for return types. This follows the Liskov Substitution Principle, making the API more flexible by accepting any mapping-like objects (dict, OrderedDict, custom mappings) while preserving backward compatibility.
Key changes:
- Updated all
put()andput_many()method signatures across protocols, stores, wrappers, and adapters to acceptMapping[str, Any] - Added
dict()conversions in encryption, compression, and limit_size wrappers where concrete dict objects are required - Modified
ManagedEntry.valuefield and return value serialization to ensure dict output
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| key_value/aio/protocols/key_value.py | Updated protocol signatures for put operations to accept Mapping |
| key_value/aio/stores/base.py | Modified base store to accept Mapping inputs and return dict outputs |
| key_value/aio/wrappers/*.py | Updated all async wrapper implementations to accept Mapping parameters |
| key_value/aio/adapters/raise_on_missing/adapter.py | Updated adapter to accept Mapping in put operations |
| key_value/sync/code_gen/**/*.py | Synchronized changes to generated sync library |
| key_value/shared/utils/managed_entry.py | Changed ManagedEntry.value field type and ensured dict serialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) -> None: | ||
| filtered_keys: list[str] = [] | ||
| filtered_values: list[dict[str, Any]] = [] | ||
| filtered_values: list[Mapping[str, Any]] = [] |
There was a problem hiding this comment.
The type annotation should remain list[dict[str, Any]] since the values are passed through unchanged to the underlying store. The Mapping type is only needed for the input parameter, not for internal storage.
| ) -> None: | ||
| filtered_keys: list[str] = [] | ||
| filtered_values: list[dict[str, Any]] = [] | ||
| filtered_values: list[Mapping[str, Any]] = [] |
There was a problem hiding this comment.
The type annotation should remain list[dict[str, Any]] since the values are passed through unchanged to the underlying store. The Mapping type is only needed for the input parameter, not for internal storage.
|
@claude please thoroughly review this pull request, are there any other fields in the interface we should change to a more generic type like the list of keys? What do you think |
|
Claude finished @strawgate's task —— View job Comprehensive PR Review: Mapping RefactorOverall Assessment: ✅ Excellent WorkThis PR successfully implements the Liskov Substitution Principle by switching input parameters from Type Checking: ✅ Passes ( Key Findings✅ What's Done Well
🤔 Copilot's Feedback (Not Valid)Copilot suggested that
Opportunities for Additional Generic TypesYou asked: "Are there any other fields in the interface we should change to a more generic type like the list of keys?" Recommendation: Yes, but with nuanceI identified two categories of methods that use 1️⃣ Input Parameters (Keys) - Should Consider ChangingMethods that accept lists of keys as input:
Recommendation: Change to Rationale:
Example: # Current
async def get_many(self, keys: list[str], ...) -> list[dict[str, Any] | None]:
# Proposed
async def get_many(self, keys: Sequence[str], ...) -> list[dict[str, Any] | None]:2️⃣ Return Types - Should NOT ChangeMethods that return lists of keys/collections:
Recommendation: Keep as Rationale:
Detailed Analysis: Should Keys Accept Sequence?Pros ✅
Cons / Considerations
|
Switch all key input parameters in bulk operations from list[str] to Sequence[str] while keeping return types as list[str]. This follows the Liskov Substitution Principle - accept more general types as inputs, return more specific types as outputs. This change completes the pattern started in #103 (Mapping for values) and improves the API's flexibility by accepting any sequence-like object (list, tuple, etc.) as input while maintaining backward compatibility. All existing code continues to work since list is a subclass of Sequence. Changes: - Updated AsyncKeyValueProtocol with Sequence for bulk operation keys - Updated BaseStore and all wrapper implementations - Updated adapters - Updated shared utility functions - Regenerated sync library Fixes #116 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: William Easton <strawgate@users.noreply.github.com>
Switch all input parameters from
dict[str, Any]toMapping[str, Any]while keeping return types asdict[str, Any]. This follows the Liskov Substitution Principle - accept more general types as inputs, return more specific types as outputs.This change improves the API's flexibility by accepting any mapping-like object (dict, OrderedDict, custom mappings, etc.) as input while maintaining backward compatibility. All existing code continues to work since dict is a subclass of Mapping.
Changes
Testing Required
Sync library needs regeneration:
Fixes #94
🤖 Generated with Claude Code
Summary by CodeRabbit