feat: add RoutingWrapper and CollectionRoutingWrapper #123
Conversation
Add routing wrapper functionality that allows routing requests to different stores based on a routing function. This enables flexible multi-backend storage strategies. Features: - RoutingWrapper: Base wrapper with custom routing function support - CollectionRoutingWrapper: Convenience wrapper for collection-based routing - Comprehensive test coverage with 11 tests covering all operations - Support for all AsyncKeyValue protocol methods (get, put, delete, ttl, and _many variants) Use cases: - Route different collections to different backends (e.g., sessions to Redis, users to DynamoDB) - Cost optimization by routing hot/cold data to different storage tiers - Compliance requirements (e.g., PII data to encrypted stores) - Development/testing with selective mock stores Closes #114 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: William Easton <strawgate@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds async and sync routing wrappers: RoutingWrapper (routes via a RoutingFunction with default fallback) and CollectionRoutingWrapper (maps collection names to stores). Exposes these in package init files, adds tests for routing behavior, and updates various lint/type-check annotations and minor store safeguards across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RW as RoutingWrapper
participant RF as routing_function
participant Store as ResolvedStore
participant Default as default_store
Client->>RW: operation(key, collection)
RW->>RF: routing_function(collection)
alt RF returns a store
RF-->>RW: Store
RW->>Store: delegate operation(...)
Store-->>RW: result
else RF returns None
RF-->>RW: None
RW->>Default: delegate operation(...)
Default-->>RW: result
end
RW-->>Client: result
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@claude please fix up this pull request so it's ready to go |
|
Claude finished @strawgate's task —— View job PR Ready for ReviewI've successfully fixed the issues in this PR and verified everything works correctly. Issues Found and Fixed
Verification Results
SummaryThe async routing wrapper implementation is now complete and correct:
The synchronous wrapper will need to be generated separately using Changes Pushed
--- | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_routing.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/__init__.py (2)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(5-55)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (1)
RoutingWrapper(12-115)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (1)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(5-55)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (9)
RoutingWrapper(12-115)put(91-93)get(71-73)put_many(96-105)get_many(76-78)delete(108-110)delete_many(113-115)ttl(81-83)ttl_many(86-88)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (2)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (2)
RoutingWrapper(12-115)get(71-73)
🔇 Additional comments (2)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/__init__.py (1)
1-4: Public API export looks good.key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (1)
70-115: Delegation and error handling look correct.
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py
Outdated
Show resolved
Hide resolved
Update RoutingWrapper to use correct type signatures: - Change keys parameters from list[str] to Sequence[str] (per #119) - Change put_many ttl from Sequence to single SupportsFloat (per #121) - Update test cases to match new put_many signature All 11 routing wrapper tests pass successfully. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
@claude please consider the PR feedback and make revisions if necessary and then do a thorough line by line review |
|
Claude finished @strawgate's task —— View job PR Review Complete
Executive SummaryThis PR successfully implements routing wrapper functionality for the py-key-value library. The implementation is solid and ready to merge with minor documentation and test improvements applied. Changes Made
CodeRabbit Feedback AssessmentI reviewed all 7 CodeRabbit suggestions and found that most were either incorrect or not aligned with this codebase's patterns: ❌ Rejected Suggestions
✅ Applied Suggestions
Comprehensive Code Review
|
…rtions - Remove misleading reference to 'key patterns' in routing wrapper docstring since the routing function only receives the collection name, not keys - Add explicit TTL validation in ttl_many test to ensure returned values are not None and are positive - Split compound assertions to comply with PT018 lint rule Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
Skipped: Cannot respond to another bot. |
1 similar comment
|
Skipped: Cannot respond to another bot. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (1)
240-266: Add edge-case test: None collection without default should raise.Covers the negative path when no default_store is configured.
class TestCollectionRoutingWrapper: @@ assert await dynamo_store.get(key="id123", collection="sessions") is None + + async def test_collection_map_none_without_default_raises(self): + wrapper = CollectionRoutingWrapper(collection_map={"sessions": MemoryStore()}) + with pytest.raises(ValueError, match="No store found for collection"): + await wrapper.get(key="k", collection=None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_routing.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (1)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(5-55)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (9)
RoutingWrapper(12-115)put(91-93)get(71-73)put_many(96-105)get_many(76-78)delete(108-110)delete_many(113-115)ttl(81-83)ttl_many(86-88)
🔇 Additional comments (2)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (1)
12-115: Routing and delegation look solid.Store resolution, default fallback, and method delegation match AsyncKeyValue; error messaging is clear. Doc wording corrected.
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (1)
1-2: Review comment is incorrect: wrong pytest marker suggested.The review recommends
pytest.mark.anyio, but the project usespytest-asyncio>=1.2.0(confirmed in dependencies), not pytest-anyio. The correct marker should bepytest.mark.asyncio.The suggestion should be:
import pytest +pytestmark = pytest.mark.asyncioNot
pytest.mark.anyioas currently stated.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (1)
15-15: Doc fixes: function signature and exceptions.
- The routing function only receives collection, not key.
- _get_store never raises here; default_store is required and used as fallback.
Apply:
- The routing function receives the collection name and key and returns the appropriate store. + The routing function receives the collection name and returns the appropriate store.- Raises: - ValueError: If no store is found for the collection and no default store is configured. + Raises: + None directly; underlying store methods may raise.Also applies to: 57-59
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (1)
1-7: Add pytest.mark.asyncio module marker.Per previous review feedback, the repository uses pytest-asyncio without
asyncio_mode=autoconfigured. Async test methods require explicit markers. Add a module-level marker after the pytest import:import pytest + +pytestmark = pytest.mark.asyncio + from dirty_equals import IsFloat
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
README.md(2 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-aio/tests/stores/base.py(2 hunks)key-value/key-value-aio/tests/stores/wrappers/test_fallback.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_routing.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-sync/src/key_value/sync/wrappers/routing/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/base.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown files must pass markdownlint with an 80-character line length limit (excluding code blocks and tables)
Files:
README.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-aio/tests/stores/base.pykey-value/key-value-aio/tests/stores/wrappers/test_fallback.pykey-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.pykey-value/key-value-sync/tests/code_gen/stores/base.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.pykey-value/key-value-aio/tests/stores/wrappers/test_routing.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/__init__.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.pykey-value/key-value-sync/src/key_value/sync/wrappers/routing/__init__.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.pykey-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Files:
key-value/key-value-sync/tests/code_gen/stores/base.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/__init__.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.pykey-value/key-value-sync/src/key_value/sync/wrappers/routing/__init__.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py
🧬 Code graph analysis (12)
key-value/key-value-aio/tests/stores/base.py (1)
key-value/key-value-sync/tests/code_gen/stores/base.py (3)
test_models_put_get(56-60)store(27-27)test_get_large_put_get(68-70)
key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (1)
key-value/key-value-aio/src/key_value/aio/wrappers/fallback/wrapper.py (3)
get(46-50)put(74-81)ttl(60-64)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (10)
RoutingWrapper(15-104)_get_store(47-62)get(65-67)get_many(70-72)ttl(75-77)ttl_many(80-82)put(85-87)put_many(90-94)delete(97-99)delete_many(102-104)key-value/key-value-aio/tests/stores/wrappers/test_routing.py (3)
default_store(30-31)store(34-44)store(121-128)
key-value/key-value-sync/tests/code_gen/stores/base.py (1)
key-value/key-value-aio/tests/stores/base.py (3)
test_models_put_get(58-62)store(30-30)test_get_large_put_get(70-72)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.py (2)
key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (2)
get(15-17)put(19-21)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (3)
get(65-67)put(85-87)ttl(75-77)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (4)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/tests/stores/base.py (2)
store(30-30)BaseStoreTests(24-239)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(8-58)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (6)
RoutingWrapper(12-110)put(86-88)get(66-68)get_many(71-73)delete(103-105)ttl(76-78)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/__init__.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(11-54)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (1)
RoutingWrapper(15-104)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (4)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-sync/tests/code_gen/stores/base.py (2)
store(27-27)BaseStoreTests(21-237)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(11-54)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (6)
RoutingWrapper(15-104)put(85-87)get(65-67)get_many(70-72)delete(97-99)ttl(75-77)
key-value/key-value-sync/src/key_value/sync/wrappers/routing/__init__.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(11-54)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (1)
RoutingWrapper(15-104)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (2)
RoutingWrapper(15-104)get(65-67)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (10)
RoutingWrapper(12-110)_get_store(48-63)get(66-68)get_many(71-73)ttl(76-78)ttl_many(81-83)put(86-88)put_many(91-100)delete(103-105)delete_many(108-110)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (3)
default_store(33-34)store(37-47)store(118-121)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (2)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (2)
RoutingWrapper(12-110)get(66-68)
⏰ 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). (6)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (20)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.py (1)
18-24: LGTM — cleaner signatures without inline suppressions.Relying on project-level lint config/pyright is fine here.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/__init__.py (1)
1-7: LGTM! Auto-generated routing wrapper exports are correct.The file correctly re-exports
CollectionRoutingWrapper,RoutingFunction, andRoutingWrapperfrom the code-gen routing modules, establishing the public API surface for sync routing wrappers.README.md (2)
299-299: LGTM! Clear documentation for CollectionRoutingWrapper.The table entry effectively describes the collection-based routing pattern with a practical example showing how to map collections to different stores.
310-310: LGTM! Clear documentation for RoutingWrapper.The table entry effectively describes the function-based routing pattern with a concise lambda example demonstrating custom routing logic.
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (5)
24-52: LGTM! Well-structured test fixtures.The fixtures correctly set up isolated store instances with a routing function that maps specific collections to dedicated stores and falls back to default_store for unmapped collections. The
store_with_datafixture seeds appropriate test data across all three collections.
53-75: LGTM! Comprehensive routing isolation test.The test thoroughly validates that routing directs operations to the correct stores, ensuring data isolation across collections. Both single-key and batch operations are verified for all three routing scenarios (mapped to first store, mapped to second store, and fallback to default).
76-100: LGTM! Thorough delete routing validation.The test comprehensively verifies that delete operations are correctly routed to the appropriate stores. For each collection, it confirms the deletion affects only the targeted store and leaves other stores unchanged, ensuring proper isolation.
102-117: LGTM! TTL routing validation is correct.The test verifies that TTL operations are properly routed to the correct stores and that TTL values are preserved approximately as expected. The use of
IsFloat(approx=...)appropriately handles minor timing variations.
119-128: LGTM! Excellent test reuse pattern.By inheriting from
TestRoutingWrapperand overriding only thestorefixture to useCollectionRoutingWrapper, this class efficiently validates that the collection-map-based routing behaves identically to the custom function-based routing. This ensures consistency between the two routing approaches.key-value/key-value-sync/src/key_value/sync/wrappers/routing/__init__.py (1)
1-7: LGTM! Auto-generated sync wrapper exports are correct.The file correctly re-exports routing wrappers from the code-gen path, mirroring the pattern used throughout the sync library to expose the public API.
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (4)
1-6: LGTM! Clean imports.The imports are minimal and appropriate:
Mappingfor flexible input types,MappingProxyTypefor immutability, and the necessary protocol and base wrapper classes.
8-33: LGTM! Excellent class documentation.The docstring clearly explains the purpose of collection-based routing and provides a practical example demonstrating routing sessions to Redis, users to DynamoDB, cache to memory, with a disk fallback for unmapped collections.
35-46: LGTM! Well-documented constructor signature.The constructor accepts
Mapping[str, AsyncKeyValue](broad input type) and requires adefault_store. The docstring clearly explains each parameter's role.
47-58: LGTM! Correct implementation addressing past feedback.The implementation properly:
- Freezes
collection_mapusingMappingProxyTypeto prevent accidental mutation (line 47)- Defines
route_by_collectionto return only mapped stores orNone(lines 49-53)- Delegates default fallback logic to the base
RoutingWrapper(lines 55-58)All past review suggestions have been correctly incorporated.
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (6)
1-10: LGTM! Properly generated sync test imports.The auto-generated test file correctly imports from the sync code-gen path and includes all necessary test dependencies.
27-55: LGTM! Well-structured sync test fixtures.The fixtures correctly set up isolated synchronous store instances with appropriate routing logic. The
store_with_datafixture properly seeds test data across all three collections without async/await (correctly generated for sync).
56-74: LGTM! Comprehensive sync routing test.The test thoroughly validates routing behavior across three stores, ensuring proper data isolation. The
# pyright: ignore[reportUnusedParameter]comment on line 58 is appropriate—thedefault_storefixture must be injected for proper test setup even though it's not directly referenced in the test body.
75-100: LGTM! Thorough sync delete routing test.The test comprehensively verifies that delete operations correctly route to the appropriate stores and maintain isolation across collections.
101-114: LGTM! Correct sync TTL routing test.The test verifies TTL operations are properly routed with appropriate approximate matching for TTL values using
IsFloat(approx=...).
116-121: LGTM! Effective test inheritance pattern.The
TestCollectionRoutingWrapperclass efficiently reuses the parent test suite by overriding only the store fixture, ensuring consistency between custom function-based and collection-map-based routing.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (1)
1-4: Add pytest.mark.asyncio module marker (confirmed missing in previous review).Per the past review, pytest-asyncio requires explicit markers when
asyncio_mode=autois not configured. Add a module-level marker so async tests execute properly:import pytest +pytestmark = pytest.mark.asyncio + from dirty_equals import IsFloat
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
key-value/key-value-aio/pyproject.toml(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_routing.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-aio/tests/stores/wrappers/test_routing.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Files:
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py
🧬 Code graph analysis (2)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (4)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/tests/stores/base.py (2)
store(30-30)BaseStoreTests(24-239)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(8-58)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (6)
RoutingWrapper(12-110)put(86-88)get(66-68)get_many(71-73)delete(103-105)ttl(76-78)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (4)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-sync/tests/code_gen/stores/base.py (2)
store(27-27)BaseStoreTests(21-237)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(11-54)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (6)
RoutingWrapper(15-104)put(85-87)get(65-67)get_many(70-72)delete(97-99)ttl(75-77)
🪛 GitHub Actions: Run Tests
key-value/key-value-aio/tests/stores/wrappers/test_routing.py
[error] 56-56: uv run basedpyright .: Method "test_not_unbounded" overrides class "BaseStoreTests" in an incompatible manner. Parameter 2 type mismatch: base parameter is type "BaseStore", override parameter is type "RoutingWrapper". (reportIncompatibleMethodOverride)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py
[error] 59-59: Method "test_not_unbounded" overrides class "BaseStoreTests" in an incompatible manner. Parameter 2 type mismatch: base parameter is type "BaseStore", override parameter is type "RoutingWrapper". "BaseStore" is not assignable to "RoutingWrapper" (reportIncompatibleMethodOverride).
🪛 GitHub Check: static_analysis (key-value/key-value-aio)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py
[failure] 56-56:
Method "test_not_unbounded" overrides class "BaseStoreTests" in an incompatible manner
Parameter 2 type mismatch: base parameter is type "BaseStore", override parameter is type "RoutingWrapper"
"BaseStore" is not assignable to "RoutingWrapper" (reportIncompatibleMethodOverride)
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py
[failure] 59-59:
Method "test_not_unbounded" overrides class "BaseStoreTests" in an incompatible manner
Parameter 2 type mismatch: base parameter is type "BaseStore", override parameter is type "RoutingWrapper"
"BaseStore" is not assignable to "RoutingWrapper" (reportIncompatibleMethodOverride)
🔇 Additional comments (12)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (7)
1-11: LGTM! File header and imports are correct.The auto-generated warning is clear, and all necessary imports are present for testing the routing wrappers.
13-26: LGTM! Test constants are well-organized.The constants clearly define test data for three routing scenarios (first collection, second collection, and unmapped collection), which aligns with the routing test coverage.
28-56: LGTM! Fixtures are well-structured.The fixtures properly set up isolated test stores and implement collection-based routing logic. The
store_with_datafixture correctly seeds data across all three routing scenarios.
61-79: LGTM! Comprehensive routing validation.The test thoroughly validates that routing correctly directs operations to the appropriate underlying stores. The fixture dependency pattern (where
store_with_datatriggers setup even without direct reference) is a standard pytest approach.
80-105: LGTM! Delete operations are thoroughly tested.The test validates that delete operations correctly route to the target store and maintains isolation across stores. Each routing scenario (first, second, and unmapped collections) is tested independently.
106-119: LGTM! TTL routing is properly tested.The test validates that TTL operations route correctly and uses appropriate floating-point comparison with
IsFloat(approx=...)for TTL values. Different TTL values for each key ensure routing independence.
121-126: LGTM! CollectionRoutingWrapper test reuses logic effectively.The subclass correctly tests
CollectionRoutingWrapperby inheriting all test methods and only overriding the fixture to use the collection-map-based wrapper. This validates that both routing implementations behave identically.key-value/key-value-aio/tests/stores/wrappers/test_routing.py (5)
10-23: LGTM: Clear test constants.The test constants are well-organized with clear naming patterns, making the test assertions easy to follow.
25-53: LGTM: Well-structured test fixtures.The fixtures properly set up the routing infrastructure with distinct stores for different collections and pre-populate test data across all routing paths.
81-106: LGTM: Comprehensive delete routing validation.The test properly verifies that delete operations route to the correct store and that data isolation is maintained across stores.
107-122: LGTM: TTL routing validation is correct.The test properly validates that TTL operations route to the appropriate stores and return expected TTL values using appropriate fuzzy matching.
124-133: LGTM: Excellent test reuse pattern.The subclass approach allows full test coverage of
CollectionRoutingWrapperby inheriting all parent tests while only changing the fixture implementation. This validates that both routing implementations provide identical behavior.
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
1-3: Do not edit this generated file directly.This file is auto-generated from the source file in
key-value/key-value-aio/. According to the coding guidelines and the warning at the top of this file, changes should be made to the original source file and then regenerated usingmake codegen, not edited here directly. Any changes made to this file will be overwritten during the next code generation run.Please revert the changes to this file and ensure the corresponding changes are applied to the source file at
key-value/key-value-aio/src/key_value/aio/stores/memory/store.pyinstead, then regenerate this sync version.Based on coding guidelines.
scripts/build_sync_library.py (1)
125-129: Consider removing the unusedfilepathparameter or document its purpose.The
filepathparameter is suppressed with# noqa: ARG001because it's never used in the function body. While the return value addition is correct, the unused parameter should be addressed:
- If
filepathis reserved for future use (e.g., error messages or debugging), document this intent in a comment.- Otherwise, remove the parameter to simplify the interface.
Regarding the
# noqa: RET504on line 129, you could avoid this suppression by refactoring to return directly without the assignment:def async_to_sync(tree: ast.AST, filepath: Path | None = None) -> ast.AST: # noqa: ARG001 tree = BlanksInserter().visit(tree) tree = RenameAsyncToSync().visit(tree) - tree = AsyncToSync().visit(tree) - return tree # noqa: RET504 + return AsyncToSync().visit(tree)However, the current pattern is consistent and acceptable if you prefer the step-by-step assignment style.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
1-3: Critical: Do not edit auto-generated files directly.This file is auto-generated (as stated in the header) and must not be edited directly. The coding guidelines explicitly state: "Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen."
The change removing the type-ignore directive should only be made in the async source file (
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py), then this sync version should be regenerated usingmake codegen.As per coding guidelines.
Revert this manual change and regenerate the file:
#!/bin/bash # Description: Revert manual changes and regenerate sync version from async source # Navigate to project root cd key-value # Regenerate the sync code from async source make codegenAlso applies to: 91-91
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
58-64: Replace pyright ignore with a precise castAvoid
# pyright: ignore[reportAny]under strict mode. Cast the return to the expected shape instead.Apply this diff to the changed line:
- return self._type_adapter.dump_python(value, mode="json") # pyright: ignore[reportAny] + return cast(dict[str, Any], self._type_adapter.dump_python(value, mode="json"))And add
castto imports (outside this hunk):-from typing import Any, Generic, SupportsFloat, TypeVar, get_origin +from typing import Any, Generic, SupportsFloat, TypeVar, get_origin, castAs per coding guidelines (Basedpyright strict).
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
58-64: Fix the pyright ignore in the AIO source and regenerateThe source file at
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.pyline 63 contains the# pyright: ignore[reportAny]comment. Replace it with a typed cast and add the import:
- Update imports at line 2:
-from typing import Any, Generic, SupportsFloat, TypeVar, get_origin +from typing import Any, Generic, SupportsFloat, TypeVar, get_origin, cast
- Update line 63:
- return self._type_adapter.dump_python(value, mode="json") # pyright: ignore[reportAny] + return cast(dict[str, Any], self._type_adapter.dump_python(value, mode="json"))
- Run
make codegento regenerate the sync version.key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
37-37: Remove unused_sessionclass attribute.The
_sessionattribute is declared but never assigned or used anywhere in the code. The session is created as a local variable on line 104 and the client is stored in_raw_clientinstead. This is dead code that should be removed to maintain code clarity.- _session: aioboto3.Session # pyright: ignore[reportAny] _table_name: str _endpoint_url: str | None _raw_client: Any # DynamoDB client from aioboto3
104-109: Remove unused_sessionattribute and add missing type stub dependency.Two critical issues need fixing:
Line 37: Remove the unused
_sessionclass attribute entirely. It's declared but never assigned or referenced anywhere in the code.Line 104 & dependencies: Removing the
# pyright: ignore[reportAny]comment requires thetypes-aioboto3package for proper type checking. Add"types-aioboto3"to thedynamodbextra inpyproject.toml:dynamodb = ["aioboto3>=13.3.0", "types-aiobotocore-dynamodb>=2.16.0", "types-aioboto3"]Without this dependency, Basedpyright in strict mode will still report
reportAnyerrors on theaioboto3.Session()call since aioboto3 lacks native type annotations.
♻️ Duplicate comments (4)
key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (1)
15-23: LGTM: Override decorators added as previously requested.The
@overridedecorators onFailingStore.getandFailingStore.putmake the overrides explicit and enable proper type checking in strict Basedpyright mode.key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (1)
38-60: Fix type incompatibility in the original source file.The
test_not_unboundedmethod declares parameter typestore: BaseStore(line 60), but the fixture at line 39 returnsRoutingWrapper.RoutingWrapperis not assignable toBaseStore, causing pipeline failures flagged in past reviews.Apply this fix in the original async source file
key-value/key-value-aio/tests/stores/wrappers/test_routing.py:@override @pytest.mark.skip(reason="RoutingWrapper is unbounded") - def test_not_unbounded(self, store: BaseStore): ... + def test_not_unbounded(self, store): ...Removing the type annotation allows pytest to inject the fixture without type checking conflicts. As per coding guidelines.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (1)
47-62: Fix misleading docstring in the original source file.The docstring at lines 56-58 claims "_get_store" raises a
ValueErrorwhen no store is found, but the implementation at lines 59-62 silently returnsdefault_storeinstead. Sincedefault_storeis required in__init__(line 36), this exception can never occur.Update the docstring in the original async source file (this is auto-generated) to accurately reflect the behavior:
def _get_store(self, collection: str | None) -> KeyValue: """Get the appropriate store for the given collection. Args: collection: The collection name to route. Returns: The KeyValue store to use for this collection. - - Raises: - ValueError: If no store is found for the collection and no default store is configured. """key-value/key-value-aio/tests/stores/wrappers/test_routing.py (1)
1-10: Add pytest.mark.asyncio module marker to enable async test execution.The repository uses pytest-asyncio without
asyncio_mode=autoconfigured. Async test methods require explicit markers to run. Add a module-level marker at the top of the file:import pytest + +pytestmark = pytest.mark.asyncio + from dirty_equals import IsFloatAs per past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
.devcontainer/devcontainer.json(2 hunks).gitignore(1 hunks)Makefile(1 hunks)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/base.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-aio/tests/stores/base.py(3 hunks)key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py(1 hunks)key-value/key-value-aio/tests/stores/keyring/test_keyring.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_fallback.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_routing.py(1 hunks)key-value/key-value-shared/src/key_value/shared/code_gen/gather.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/base.py(3 hunks)key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/keyring/test_keyring.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py(1 hunks)pyproject.toml(2 hunks)scripts/build_sync_library.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-aio/tests/stores/wrappers/test_fallback.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.pykey-value/key-value-aio/src/key_value/aio/stores/disk/store.pykey-value/key-value-aio/tests/stores/wrappers/test_routing.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.pykey-value/key-value-shared/src/key_value/shared/code_gen/gather.pykey-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.pykey-value/key-value-aio/tests/stores/base.pykey-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.pykey-value/key-value-sync/tests/code_gen/stores/keyring/test_keyring.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.pykey-value/key-value-aio/src/key_value/aio/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.pykey-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.pykey-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.pykey-value/key-value-sync/tests/code_gen/stores/base.pykey-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.pykey-value/key-value-aio/tests/stores/keyring/test_keyring.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/base.pyscripts/build_sync_library.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.pykey-value/key-value-aio/src/key_value/aio/stores/memory/store.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.pykey-value/key-value-sync/tests/code_gen/stores/keyring/test_keyring.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.pykey-value/key-value-sync/tests/code_gen/stores/base.pykey-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py
key-value/key-value-aio/src/key_value/aio/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
key-value/key-value-aio/src/key_value/aio/stores/**/*.py: Async store implementations must implement the AsyncKeyValue protocol from key_value/aio/protocols/key_value.py
Store backends must store and retrieve ManagedEntry wrappers (with value, creation timestamp, and TTL), not raw values
Files:
key-value/key-value-aio/src/key_value/aio/stores/disk/store.pykey-value/key-value-aio/src/key_value/aio/stores/base.pykey-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.pykey-value/key-value-aio/src/key_value/aio/stores/memory/store.py
🧠 Learnings (1)
📚 Learning: 2025-10-26T13:44:25.428Z
Learnt from: CR
PR: strawgate/py-key-value#0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T13:44:25.428Z
Learning: Applies to key-value/key-value-aio/src/key_value/aio/stores/**/*.py : Async store implementations must implement the AsyncKeyValue protocol from key_value/aio/protocols/key_value.py
Applied to files:
key-value/key-value-aio/tests/stores/base.pykey-value/key-value-sync/tests/code_gen/stores/base.py
🧬 Code graph analysis (19)
key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (3)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
get(111-137)put(193-204)ttl(148-157)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (3)
get(9-24)put(39-49)ttl(26-37)key-value/key-value-aio/src/key_value/aio/wrappers/fallback/wrapper.py (3)
get(46-50)put(74-81)ttl(60-64)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
_memory_cache_ttu(46-55)MemoryCacheEntry(25-43)_memory_cache_getsizeof(58-60)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
get(111-137)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
get(84-92)
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (5)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/tests/stores/base.py (3)
store(31-31)BaseStoreTests(25-243)test_not_unbounded(200-208)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (5)
MemoryStore(105-180)put(94-95)get(84-92)keys(100-102)delete(97-98)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(8-49)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (6)
RoutingWrapper(12-110)put(86-88)get(66-68)get_many(71-73)delete(103-105)ttl(76-78)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (5)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-sync/tests/code_gen/stores/base.py (3)
store(28-28)BaseStoreTests(22-240)test_not_unbounded(197-205)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (5)
MemoryStore(100-169)put(89-90)get(79-87)keys(95-97)delete(92-93)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
CollectionRoutingWrapper(11-45)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (6)
RoutingWrapper(15-104)put(85-87)get(65-67)get_many(70-72)delete(97-99)ttl(75-77)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (2)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
ping_elasticsearch(29-33)
key-value/key-value-aio/tests/stores/base.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (3)
AsyncKeyValueProtocol(6-115)put(39-49)get(9-24)key-value/key-value-sync/tests/code_gen/stores/base.py (2)
test_store(31-33)store(28-28)key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
BaseStore(28-280)put(193-204)get(111-137)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (10)
RoutingWrapper(15-104)_get_store(47-62)get(65-67)get_many(70-72)ttl(75-77)ttl_many(80-82)put(85-87)put_many(90-94)delete(97-99)delete_many(102-104)key-value/key-value-aio/tests/stores/wrappers/test_routing.py (3)
default_store(32-33)store(36-46)store(127-134)
key-value/key-value-sync/tests/code_gen/stores/keyring/test_keyring.py (1)
key-value/key-value-sync/tests/code_gen/stores/base.py (2)
test_get_large_put_get(71-73)store(28-28)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
get(114-135)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
get(79-87)
key-value/key-value-aio/src/key_value/aio/stores/base.py (7)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
_setup_collection(129-130)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
_setup_collection(77-78)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
_setup_collection(124-125)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
_setup_collection(107-108)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
_setup_collection(110-111)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
_setup_collection(123-138)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_setup_collection(130-145)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (10)
RoutingWrapper(12-110)_get_store(48-63)get(66-68)get_many(71-73)ttl(76-78)ttl_many(81-83)put(86-88)put_many(91-100)delete(103-105)delete_many(108-110)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (3)
default_store(35-36)store(39-49)store(124-127)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (2)
RoutingWrapper(12-110)get(66-68)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (2)
CollectionRoutingWrapper(11-45)route_by_collection(39-43)
key-value/key-value-sync/tests/code_gen/stores/base.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValueProtocol(9-108)key-value/key-value-aio/tests/stores/base.py (2)
test_store(34-36)store(31-31)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (2)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (1)
ping_elasticsearch(26-30)
key-value/key-value-aio/tests/stores/keyring/test_keyring.py (1)
key-value/key-value-aio/tests/stores/base.py (2)
test_get_large_put_get(74-76)store(31-31)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
_setup_collection(124-125)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
_setup_collection(110-111)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
_setup_collection(130-145)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (3)
_memory_cache_ttu(45-54)MemoryCacheEntry(28-42)_memory_cache_getsizeof(57-59)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (2)
RoutingWrapper(15-104)get(65-67)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (2)
CollectionRoutingWrapper(8-49)route_by_collection(40-44)
🔇 Additional comments (38)
.gitignore (1)
9-11: LGTM! MacOS-specific artifacts are now properly ignored.The addition of
.DS_Storeis a sensible hygiene improvement. The placement and formatting are consistent with the rest of the file.Makefile (1)
72-80: Inconsistency between PROJECT-specific and all-packages sync commands.Line 75 uses
--locked --group dev, but line 78 uses only--group dev. This discrepancy could lead to different dependency resolutions across the two code paths:
- The
--lockedflag ensures reproducible lockfile-based syncing and should typically be consistent across both paths.- If other dependency groups (e.g., main, optional) exist, restricting only to
--group devmay skip essential dependencies.Please verify:
- Should
--lockedbe added to line 78 for reproducibility consistency?- Is restricting to
--group devintentional, or should all dependency groups be included?For reference, the PROJECT-specific path (line 75) is:
@cd $(PROJECT) && uv sync --locked --group devpyproject.toml (1)
70-71: ✓ Per-file ignores correctly scoped to tests.ARG001 and ARG002 are now scoped to test files only, removing the overly broad global suppression. This aligns with the earlier feedback and ensures production code maintains stricter linting enforcement.
key-value/key-value-shared/src/key_value/shared/code_gen/gather.py (1)
13-13: The review comment is incorrect—the noqa suppression is correct as-is.The distinction between ARG001 and ARG002 is based on function type, not argument kind. An unused **kwargs in a free function reports ARG001, while an unused **kwargs in an instance method reports ARG002. Since
gather()is a plain function, the# noqa: ARG001comment correctly suppresses the diagnostic for the unused**kwargsparameter. No change is needed.key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (2)
42-42: Good refactoring to use ES_PORT constant.Using the
ES_PORTvariable instead of the hard-coded port number improves maintainability and consistency.
44-44: Improved test reliability with longer wait time.Increasing the wait time to 2 seconds helps ensure Elasticsearch is fully ready before running tests, reducing potential flakiness.
.devcontainer/devcontainer.json (2)
8-9: Verify the connection between devcontainer infrastructure changes and the routing wrapper PR.The PR summary describes routing wrapper functionality for key-value stores, but the devcontainer changes (github-cli, git, --network=host, Docker socket mount) suggest infrastructure or CI/integration testing improvements. The relationship between these changes and the PR objectives is unclear.
Please clarify:
- Are these devcontainer changes necessary to support the routing wrapper implementation or its tests?
- Should these infrastructure changes be documented in the PR description or moved to a separate PR?
- Are there specific testing or integration scenarios that require network host access and Docker socket mounting?
14-16: Docker socket mount is necessary and justified—integration tests require it.The mounting of
/var/run/docker.sockis essential for this codebase's test suite. Multiple integration tests spawn external service containers (Redis, MongoDB, Elasticsearch, Vault, Valkey) using the Docker Python SDK. Tests properly skip when Docker is unavailable.The concern about privileged access is valid, but the necessity is clear and intentional. However, adding an inline comment to
.devcontainer/devcontainer.jsonexplaining this dependency would improve maintainability and clarify the purpose to future maintainers.key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (1)
88-88: Type-ignore removal is sound—diskcache-stubs are available.The removal of the pyright ignore directive is justified. The codebase includes
diskcache-stubs>=5.6.3.6.20240818in dependencies, which provides proper type annotations for the tuple unpacking at line 88. The code structure is correct:expire_epochis pre-declared with typefloat | Noneon line 86, matching the return type ofself._cache.get(key=combo_key, expire_time=True).Verify locally that Basedpyright (strict mode) passes without errors on this file:
basedpyright key-value/key-value-aio/src/key_value/aio/stores/disk/store.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
42-42: Good: explicit TypeAdapter annotation clarifies intentThe explicit
_type_adapter: TypeAdapter[T]improves readability and type safety. LGTM.key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
42-42: Good: mirrors AIO adapter’s typed TypeAdapterKeeping
_type_adapter: TypeAdapter[T]consistent across AIO/Sync is correct.key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
74-75: LGTM: Pyright suppression correctly removed.The
_setup_collectionmethod is abstract and thecollectionparameter is used by all subclass implementations (MemoryStore, DiskStore, MongoDB), so the pyright ignore directive was unnecessary.key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
77-78: LGTM: Docstring added correctly.The docstring clarifies the purpose of
_setup_collection. Since this is auto-generated from the async source, the change correctly propagates from the original file.key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (2)
46-55: LGTM: Pyright suppression removal is appropriate.The
_memory_cache_ttufunction properly uses all its parameters, so the removal of the pyright ignore directive is correct.
58-60: LGTM: Linting directive switched to Ruff.The switch from
pyright: ignore[reportUnusedParameter]to# noqa: ARG001aligns with the project's use of Ruff for linting. The suppression is appropriate since_memory_cache_getsizeofintentionally ignores thevalueparameter per the cachetools API.key-value/key-value-aio/tests/stores/base.py (2)
33-36: LGTM: Protocol compliance test added.The
test_storemethod verifies that store implementations correctly implementAsyncKeyValueProtocol, which is a useful validation for the new routing wrappers and existing stores.
62-74: LGTM: Pyright suppressions removed.The inline
# pyright: ignore[reportUnusedParameter, reportUnusedParameter]comments (with duplicate codes) have been removed. The test parameters are consumed by pytest's parametrize decorator, so they only appear unused to static analysis.key-value/key-value-sync/tests/code_gen/stores/wrappers/test_fallback.py (1)
18-26: LGTM: Override decorators correctly generated.The
@overridedecorators on the syncFailingStoremethods correctly mirror the async implementation, maintaining consistency across code-gen and async versions.key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (1)
62-120: LGTM: Routing tests comprehensively verify delegation.The tests verify that:
- Operations route to the correct backing stores based on collection name
- Unmapped collections fall back to the default store
- Delete operations propagate correctly
- TTL operations preserve expiration across routing
key-value/key-value-sync/tests/code_gen/stores/keyring/test_keyring.py (1)
35-36: Verify change was made to the original source file.The removal of the
pyright: ignore[reportUnusedParameter]comment is correct since all parameters are used in the super() call. However, per the file header and coding guidelines, this is auto-generated code and should not be edited directly.Ensure the corresponding change was made to the original source file that generates this code.
As per coding guidelines.
key-value/key-value-aio/tests/stores/wrappers/test_routing.py (7)
11-24: LGTM!Test constants are well-defined with clear naming that makes the test intent obvious. The separation of mapped and unmapped collections facilitates comprehensive routing tests.
27-53: LGTM!Fixtures are well-structured with proper isolation. The routing function correctly returns
Nonefor unmapped collections, delegating default store fallback to the baseRoutingWrapperimplementation.
55-57: LGTM!The method signature now correctly uses
BaseStoretype, matching the base class signature. This resolves the previousreportIncompatibleMethodOverrideerror.
59-81: LGTM!Test comprehensively validates routing behavior by confirming each backing store receives only its routed collections. The
default_storeparameter is correctly used in assertions (lines 77-80), and the previously flagged unnecessary pyright ignore comment has been removed.
82-106: LGTM!Delete test comprehensively validates that delete operations route to the correct store and maintain isolation across stores.
108-122: LGTM!TTL test validates that TTL operations route correctly and preserve TTL semantics across different stores. The use of
IsFloat(approx=...)appropriately handles minor timing variations.
125-134: LGTM!Excellent test reuse via inheritance. This validates that
CollectionRoutingWrapperprovides the same routing semantics as the customrouting_functionapproach in the parent test class.key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (2)
8-24: LGTM!Class documentation clearly explains the collection-based routing pattern with a helpful example.
26-49: LGTM!Implementation correctly:
- Accepts any
Mappingtype for flexibility- Freezes the collection map with
MappingProxyTypeto prevent mutation- Delegates default store fallback to the base
RoutingWrapperby returningNonefor unmapped collectionskey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
11-45: LGTM!Auto-generated sync implementation correctly mirrors the async version, using
KeyValueprotocol instead ofAsyncKeyValue. The implementation properly usesMappingProxyTypefor immutability and delegates default fallback to the base wrapper.key-value/key-value-sync/tests/code_gen/stores/base.py (3)
17-17: LGTM!Import addition supports the new protocol compliance test.
30-33: LGTM!Protocol compliance test with appropriate timeout ensures stores correctly implement the
KeyValueProtocolinterface.
59-71: LGTM!Removal of pyright ignore comments is correct since all parameters are used in the test methods. Ensure corresponding changes were made to the original source file, as this is auto-generated code.
As per coding guidelines.
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (4)
9-9: LGTM!Clear type alias that documents the routing function contract: takes a collection name and returns either a store or
None.
12-31: LGTM!Class documentation accurately describes collection-based routing with a clear example. Previous concern about implying key-based routing has been addressed.
33-46: LGTM!Clean initialization that stores the routing function and default store. Type checking ensures the routing function signature is correct.
65-110: LGTM!All AsyncKeyValue protocol methods correctly delegate to the resolved store with consistent patterns:
- Resolve store via
_get_store(collection)- Delegate operation with all original parameters
- Return/await the result
The
@overridedecorator ensures signatures stay in sync with the protocol.key-value/key-value-aio/tests/stores/keyring/test_keyring.py (1)
32-33: Removal of unnecessary ignore directive is correct.The
jsonparameter is forwarded to the parent method on line 33, so thereportUnusedParameterignore directive was unnecessary. This cleanup appropriately aligns with the PR's type-checking directive refactoring.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (2)
43-47: Same origin/issubclass guard needed — fix upstream and regenerateThis mirrors the AIO logic; apply the AIO fix in the source adapter/template and re-run codegen. Do not edit this file directly.
After updating the source, ensure make codegen (or your generation step) reproduces the change here.
69-69: Replace suppression with cast in source; regenerateAs with AIO, prefer typing.cast over pyright ignore. Make the change in the original file so codegen reflects it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py
🧬 Code graph analysis (2)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)
🔇 Additional comments (5)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (3)
18-22: Private attributes declaration looks goodExplicit, private fields improve strict type checking and readability. No behavior change.
49-50: Defaults wired correctly_default_collection and _raise_on_validation_error are set plainly; no issues.
69-69: Add cast import and replace pyright ignore with explicit type castThe suggested fix is correct. The import statement at line 2 currently lacks
cast, and usingcast(dict[str, Any], ...)is the appropriate way to handle theAnyreturn type from Pydantic'sdump_python()method while satisfying strict type checking. Apply the provided diff as-is. Note: the sync adapter will automatically receive the same fix when regenerated, as indicated by its auto-generation notice.key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (2)
21-26: Generated fields look correctPrivate attributes mirror the AIO version; consistent and type-friendly.
49-50: Defaults assigned correctly in generated codeMatches AIO; no issues.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
1-166: CRITICAL: Do not modify generated code directly.This file is auto-generated (line 1 warning) and changes here will be overwritten by
make codegen. All modifications must be made to the original source file that generates this sync version.As per coding guidelines:
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Please revert changes to this file and apply them to the source file instead (likely in the async/aio version or the generation script).
♻️ Duplicate comments (1)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
46-46: Past review concern has been correctly addressedThe
isinstance(origin, type)guard has been properly added to prevent TypeError whenget_origin()returns non-class types liketyping.Unionortypes.UnionType. The fix matches the recommended solution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
AGENTS.md(1 hunks)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.pykey-value/key-value-aio/src/key_value/aio/stores/valkey/store.pykey-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.pykey-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown files must pass markdownlint with an 80-character line length limit (excluding code blocks and tables)
Files:
AGENTS.md
key-value/key-value-aio/src/key_value/aio/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
key-value/key-value-aio/src/key_value/aio/stores/**/*.py: Async store implementations must implement the AsyncKeyValue protocol from key_value/aio/protocols/key_value.py
Store backends must store and retrieve ManagedEntry wrappers (with value, creation timestamp, and TTL), not raw values
Files:
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py
🧠 Learnings (2)
📚 Learning: 2025-10-26T13:44:25.428Z
Learnt from: CR
PR: strawgate/py-key-value#0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T13:44:25.428Z
Learning: Applies to key-value/key-value-sync/** : Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Applied to files:
AGENTS.md
📚 Learning: 2025-10-26T13:44:25.428Z
Learnt from: CR
PR: strawgate/py-key-value#0
File: AGENTS.md:0-0
Timestamp: 2025-10-26T13:44:25.428Z
Learning: Develop async-first: make changes in the async package and regenerate the sync package with make codegen
Applied to files:
AGENTS.md
🧬 Code graph analysis (8)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(133-137)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(133-137)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (9)
get(111-137)get_many(140-145)keys(290-296)ttl(148-157)ttl_many(160-175)put(193-204)put_many(222-239)delete(258-262)delete_many(265-270)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (10)
RoutingWrapper(15-104)_get_store(50-62)get(65-67)get_many(70-72)ttl(75-77)ttl_many(80-82)put(85-87)put_many(90-94)delete(97-99)delete_many(102-104)key-value/key-value-aio/tests/stores/wrappers/test_routing.py (3)
default_store(32-33)store(36-46)store(127-134)
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (2)
RoutingWrapper(12-110)get(66-68)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (2)
CollectionRoutingWrapper(11-47)route_by_collection(41-45)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (2)
RoutingWrapper(15-104)get(65-67)key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (2)
CollectionRoutingWrapper(8-51)route_by_collection(42-46)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (10)
RoutingWrapper(12-110)_get_store(51-63)get(66-68)get_many(71-73)ttl(76-78)ttl_many(81-83)put(86-88)put_many(91-100)delete(103-105)delete_many(108-110)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_routing.py (3)
default_store(35-36)store(39-49)store(124-127)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)
🪛 LanguageTool
AGENTS.md
[style] ~109-~109: Consider shortening or rephrasing this to strengthen your wording.
Context: ...nerate. Always run make codegen after making changes to the async package. You will need to inc...
(MAKE_CHANGES)
🔇 Additional comments (8)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
18-22: LGTM: Explicit attribute declarations improve type safetyThese explicit private attribute declarations enable strict type checking and prevent attribute-not-found errors in Basedpyright strict mode.
key-value/key-value-aio/src/key_value/aio/wrappers/routing/collection_routing.py (1)
1-51: LGTM! Clean implementation with past feedback addressed.The
CollectionRoutingWrapperimplementation correctly:
- Accepts
Mapping[str, AsyncKeyValue]for flexibility (addresses past feedback)- Wraps the collection map in
MappingProxyTypefor defensive immutability (addresses past feedback)- Returns
Nonefrom the routing function for unmapped collections, letting the baseRoutingWrapperhandle the default store fallback (addresses past feedback per commit 7b1fa21)- Follows the same pattern as the sync version
The implementation is type-safe, defensive, and provides a clear convenience layer over the base
RoutingWrapper.key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py (4)
1-9: LGTM! Clear imports and type alias.The
RoutingFunctiontype alias clearly documents the routing function signature, making the API easier to understand and use.
12-49: LGTM! Clear class design with past feedback addressed.The class docstring and constructor are well-documented. The docstring correctly describes collection-based routing without implying key-based routing capabilities (addresses past feedback per commit 11f21cd). The constructor cleanly stores the routing function and default store without unnecessary validation.
51-63: LGTM! Clean routing resolution logic.The
_get_storehelper correctly resolves the target store using the routing function with a fallback to the default store. The docstring accurately describes the behavior without the previously mentionedValueError(addresses past feedback).
65-110: LGTM! Complete and correct protocol implementation.All
AsyncKeyValueprotocol methods are properly implemented with:
- Consistent
@overridedecorators- Correct
awaitusage on all async store operations- Proper parameter forwarding including collection and ttl
- Accurate return type handling for each operation
The delegation pattern is clean and straightforward, ensuring all operations are routed to the appropriate store without transformation or interference.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/collection_routing.py (1)
1-47: Auto-generated sync code is correct.This file is correctly auto-generated from the async source. The implementation properly adapts CollectionRoutingWrapper for synchronous use: it wraps the collection map in an immutable MappingProxyType and defines routing logic that delegates to the base RoutingWrapper. The example correctly shows synchronous usage without
await.As per coding guidelines, any future changes must be made to the upstream async source file and regenerated via
make codegen.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/routing/wrapper.py (1)
1-104: Auto-generated sync code is correct.This file is correctly auto-generated from the async source. The RoutingWrapper implementation properly:
- Defines RoutingFunction type alias for routing logic
- Stores routing function and default store
- Implements _get_store helper with fallback to default_store when routing returns None
- Delegates all KeyValue protocol methods to the selected store with proper parameter forwarding
The docstring correctly documents that the routing function receives only the collection name (line 18), matching the type signature, and the fallback behavior to default_store is intentional per the constructor documentation (line 45).
As per coding guidelines, any future changes must be made to the upstream async source file and regenerated via
make codegen.
| async package and regenerate. Always run `make codegen` after making changes | ||
| to the async package. You will need to include the generated code in your pull | ||
| request. Nobody will generate it for you. This also means pull requests will contain | ||
| two copies of your changes, this is intentional! |
There was a problem hiding this comment.
Fix line length constraint violation in Markdown.
Line 111 exceeds the 80-character line length limit (currently 85 characters). Additionally, the phrasing can be tightened to avoid redundancy per LanguageTool's suggestion on line 109.
Refactor the paragraph to respect the 80-character constraint while strengthening wording:
### Sync Package is Generated
**Never edit files in `key-value/key-value-sync/` directly**. Any changes
will be overwritten when `make codegen` runs. Always make changes in the
-async package and regenerate. Always run `make codegen` after making changes
-to the async package. You will need to include the generated code in your pull
-request. Nobody will generate it for you. This also means pull requests will contain
-two copies of your changes, this is intentional!
+async package and run `make codegen` to regenerate. Include the generated
+code in your pull request—it will not be generated for you. PRs will
+intentionally contain two copies of your changes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async package and regenerate. Always run `make codegen` after making changes | |
| to the async package. You will need to include the generated code in your pull | |
| request. Nobody will generate it for you. This also means pull requests will contain | |
| two copies of your changes, this is intentional! | |
| async package and run `make codegen` to regenerate. Include the generated | |
| code in your pull request—it will not be generated for you. PRs will | |
| intentionally contain two copies of your changes. |
🧰 Tools
🪛 LanguageTool
[style] ~109-~109: Consider shortening or rephrasing this to strengthen your wording.
Context: ...nerate. Always run make codegen after making changes to the async package. You will need to inc...
(MAKE_CHANGES)
🤖 Prompt for AI Agents
In AGENTS.md around lines 109 to 112, the paragraph about regenerating the async
package exceeds the 80-char limit and is slightly redundant; reword and wrap the
text into shorter lines so no line is over 80 characters, tighten phrasing by
removing redundancy (e.g., replace the current three lines with a concise
sentence such as: after making changes to the async package run `make codegen`
and include the generated code in your PR; pull requests will intentionally
contain two copies of your changes), and ensure line breaks occur at or before
80 characters.
| if self._connected_client is None: | ||
| return | ||
| await self._client.close() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Good defensive guard to prevent close errors.
The None check correctly prevents attempting to close an uninitialized client, which could happen if _close() is called before _setup() or if the store is torn down without ever connecting.
Optional style improvement:
For consistency, consider calling close() directly on _connected_client instead of going through the _client property, since you've already verified it's not None:
async def _close(self) -> None:
if self._connected_client is None:
return
- await self._client.close()
+ await self._connected_client.close()This makes the code more direct and maintains consistency with the guard check.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py around lines
119 to 121, the guard correctly checks self._connected_client is not None but
then calls await self._client.close(); update the call to await
self._connected_client.close() to be consistent with the guard and avoid any
indirection, making sure the attribute exists and is awaited the same way as
before.
| def _close(self) -> None: | ||
| if self._connected_client is None: | ||
| return | ||
| self._client.close() |
There was a problem hiding this comment.
Critical: Do not edit generated files directly.
This file is auto-generated (as stated in lines 1-3) and located in key-value/key-value-sync/, which the coding guidelines explicitly prohibit editing directly. Any changes made here will be overwritten by make codegen.
The safe-close guard logic is correct, but this change must be applied to the original source file (likely in key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py) and then regenerated using the build process.
As per coding guidelines.
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py
around lines 114 to 117, do not edit this generated file directly; the change
(ensure safe-close uses self._connected_client guard before calling
self._client.close()) must be applied in the original source (likely
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py) — update the
corresponding _close implementation there to include the guard, then run the
project codegen (make codegen) to regenerate the sync file so the fix is
preserved by the build process.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-value/key-value-aio/tests/stores/memcached/test_memcached.py (2)
22-32: Narrow exception scope in ping_memcached to avoid masking real errors.Catching
Exceptioncan hide non-connectivity bugs. Limit to connection/timeout errors.async def ping_memcached() -> bool: client = Client(host=MEMCACHED_HOST, port=MEMCACHED_PORT) try: await client.stats() - except Exception: + except (OSError, TimeoutError): return False else: return True finally: with contextlib.suppress(Exception): await client.close()
43-49: Replace magic number with defined constant for consistency.The constant
WAIT_FOR_MEMCACHED_TIMEOUT = 30is already defined at line 19; use it instead of the hardcodedtries=30on line 45. The port mapping{"11211": MEMCACHED_PORT}is correct per docker-py semantics and requires no changes.- if not await async_wait_for_true(bool_fn=ping_memcached, tries=30, wait_time=1): + if not await async_wait_for_true( + bool_fn=ping_memcached, tries=WAIT_FOR_MEMCACHED_TIMEOUT, wait_time=1 + ): msg = "Memcached failed to start" raise MemcachedFailedToStartError(msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
key-value/key-value-aio/tests/stores/memcached/test_memcached.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-aio/tests/stores/memcached/test_memcached.py
| # Memcached test configuration | ||
| MEMCACHED_HOST = "localhost" | ||
| MEMCACHED_PORT = 11211 | ||
| MEMCACHED_IMAGE = "memcached:1.6" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin the Memcached image (or allow override) to stabilize CI.
memcached:1.6 floats to the latest 1.6.x, which can change behavior over time. Pin a patch version or digest, and optionally allow an env override for local runs.
+import os
@@
-MEMCACHED_IMAGE = "memcached:1.6"
+# Allow overriding in CI/local; default remains a stable tag (adjust as needed).
+MEMCACHED_IMAGE = os.getenv("TEST_MEMCACHED_IMAGE", "memcached:1.6")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MEMCACHED_IMAGE = "memcached:1.6" | |
| import os | |
| # ... (other imports and code) | |
| # Allow overriding in CI/local; default remains a stable tag (adjust as needed). | |
| MEMCACHED_IMAGE = os.getenv("TEST_MEMCACHED_IMAGE", "memcached:1.6") |
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/memcached/test_memcached.py around line
17, the test sets MEMCACHED_IMAGE = "memcached:1.6" which floats to the latest
1.6.x; change this to a pinned image (either a specific patch tag like
"memcached:1.6.17" or a digest like "memcached@sha256:...") and add an optional
environment variable override (e.g., read MEMCACHED_IMAGE from os.environ if
present) so CI is stable but local runs can override the image.
New Features
Bug Fixes
Documentation
Tests
Chores