refactor: switch key input parameters from list to Sequence #119
Conversation
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>
📝 WalkthroughWalkthroughBatch operation method signatures across the key-value async framework are updated to accept 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 (2)
🧰 Additional context used🧬 Code graph analysis (1)key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py (2)
🔇 Additional comments (5)
Comment |
|
@claude can you review this and make sure we aren't missing anywhere where we need to turn it into a concrete list, etc? |
|
Claude finished @strawgate's task —— View job Code Review - Type Issues FoundI've reviewed the PR changes and found several type inconsistencies where 🔴 Critical Issues1. all_keys: Sequence[str] = [] # ❌ Should be list[str]
for hit in hits:
if not (key := get_first_value_from_field_in_hit(hit=hit, field="key", value_type=str)):
continue
all_keys.append(key) # ❌ Sequence doesn't have .append()2. _cursor: int
keys: Sequence[str] # ❌ Should be list[str]
_cursor, keys = await self._client.scan(cursor=0, match=pattern, count=limit)
# scan() returns list[str], not Sequence[str]3. filtered_keys: Sequence[str] = [] # ❌ Should be list[str]
filtered_values: list[Mapping[str, Any]] = []
# ...
for i, (k, v) in enumerate(zip(keys, values, strict=True)):
if self._within_size_limit(value=dict(v), collection=collection, key=k):
filtered_keys.append(k) # ❌ Sequence doesn't have .append()4. entries_to_cache_keys: Sequence[str] = [] # ❌ Should be list[str]
# ...
entries_to_cache_keys.append(key) # ❌ Line 84Same issue at line 135: entries_to_cache_keys: Sequence[str] = [] # ❌ Should be list[str]
# ...
entries_to_cache_keys.append(key) # ❌ Line 141SummaryThe PR correctly changes input parameters from RecommendationChange the following lines from
These are local variables being built up with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@claude please fix the issues you found |
|
Claude finished @strawgate's task —— View job Type Annotation Issues - Fixed ✅I've successfully fixed all 5 type annotation issues where local variables were incorrectly typed as Changes MadeFixed the following files:
Validation
Note on Sync LibraryThe sync library regeneration requires Python 3.10, but this CI environment runs Python 3.12. The sync library will be regenerated automatically by the CI pipeline which has the correct Python version configured. SummaryThe PR correctly changes input parameters from --- | |
…pend() is used Fixed type annotations for local variables that are initialized as empty lists and built up using .append(). These variables should be typed as list[str] rather than Sequence[str] since Sequence is immutable and doesn't support .append(). Changes: - elasticsearch/store.py: all_keys in _get_collection_keys - redis/store.py: keys in _get_collection_keys - limit_size/wrapper.py: filtered_keys in put_many - passthrough_cache/wrapper.py: entries_to_cache_keys in get_many and ttl_many Note: Sync library regeneration will be done in CI which has Python 3.10. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
|
@claude please thoroughly review this pr for issues |
|
Claude finished @strawgate's task —— View job Thorough PR Review - Complete ✅Review SummaryI've conducted a comprehensive review of PR #119 and found that all type issues have been correctly fixed. The refactoring from Verification Results✅ Type CheckingAll type annotations are correct with basedpyright in strict mode. ✅ Linting✅ Code Review Findings1. Input Parameters - Correctly Changed ✅
2. Local Variables - Previously Fixed ✅
These use 3. Intermediate Variables - Acceptable Pattern ✅
4. Imports - All Present ✅ 5. Shared Utilities - Updated ✅
6. Test Code - Updated ✅ 7. No Store Overrides - Safe ✅ Minor Observations1. Sync Library Regeneration Recommendation: Verify that CI successfully regenerates the sync library with these changes. 2. Intermediate Variable Precision (Optional)
This is not a bug (current code is type-safe), but would make the types more precise since list comprehensions always return Final Verdict✅ APPROVED - This PR correctly implements the refactoring from Key Strengths:
No blocking issues found. |
- Add seed parameter to MemoryStore for pre-populating data on init - Add DefaultValueWrapper to return default values for missing keys - Add default parameter to PydanticAdapter get() and get_many() methods - Add PydanticJsonWrapper to ensure JSON-serializable value conversion All features updated to use Sequence[str] for key parameters per PR #119. Fixes #115 Co-authored-by: William Easton <strawgate@users.noreply.github.com>
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>
Summary
Switch all key input parameters in bulk operations from
list[str]toSequence[str]while keeping return types aslist[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
Test Plan
Fixes #116
🤖 Generated with Claude Code
Summary by CodeRabbit