feat: require single TTL for all items in put_many #121
Conversation
This breaking change simplifies the put_many API by requiring a single TTL value for all items in a batch operation, instead of supporting per-item TTLs. Changes: - Updated AsyncKeyValueProtocol.put_many() to accept single TTL - Simplified BaseStore._prepare_put_many() to handle single TTL - Removed prepare_ttls() utility function (no longer needed) - Removed IncorrectTTLCountError exception class - Updated all wrappers and adapters to use single TTL - Updated tests to reflect new behavior - Updated README.md with new signature Rationale: - Industry standard: Django, Flask, redis-py all use single TTL - Simplifies API and reduces complexity - Most backend stores don't support per-item TTLs in batch operations - Pre-GA library, so breaking changes are acceptable For users needing different TTLs: - Make multiple put_many() calls grouped by TTL - Use individual put() calls Fixes #95 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Fixed compression and encryption wrappers - Fixed fallback, logging, passthrough_cache, prefix_collections, prefix_keys, read_only, retry, single_collection, statistics, and timeout wrappers - Updated passthrough_cache to use individual put() calls for items with different TTLs - Updated test to reflect single TTL approach - All async type checking now passes Note: Sync library needs regeneration with make codegen Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
Warning Rate limit exceeded@strawgate has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (43)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
|
@coderabbitai please review! |
|
@claude please do a very thorough line by line review for issues |
|
@strawgate: I'll review the changes in this PR right away! ✅ Actions performedReview triggered.
|
|
Claude finished @strawgate's task —— View job Thorough Line-by-Line ReviewI've completed a comprehensive review of all 43 changed files. Here are my findings: ✅ Overall AssessmentThis is a well-executed breaking change that successfully simplifies the 🔍 Detailed Findings1. ✅ Protocol & API Changes (EXCELLENT)Files reviewed:
Verdict: Clean signature change with clear documentation. The docstring correctly explains that the same TTL applies to all items in the batch. 2. ✅ Base Store Implementation (EXCELLENT)Files reviewed:
Verdict: The refactoring is clean:
3. ✅ Shared Utilities (EXCELLENT)Files reviewed:
Verdict: Clean removal of unused code:
4. ✅ Wrappers - Mostly ExcellentFiles reviewed (19 wrapper files): All wrappers correctly updated to use single TTL. Special attention to: a) PassthroughCacheWrapper ✅ (Smart refactoring)
Excellent: The wrapper now uses individual b) LimitSizeWrapper ✅
Excellent: Correctly simplified to filter keys/values without tracking per-item TTLs. The single TTL is passed through unchanged. c) TTLClampWrapper ✅
Excellent: Simplified from clamping each TTL in a sequence to clamping a single TTL. Clean refactoring. 5. ✅ Adapters (EXCELLENT)Files reviewed:
Verdict: Both adapters updated correctly with proper docstring updates explaining the single TTL behavior. 6. ✅ Tests (GOOD)Files reviewed:
Verdict: Test correctly updated from per-item TTLs to single TTL. Test name and comments updated appropriately. 7. ✅ Sync Library Codegen Consistency (EXCELLENT)Spot-checked files:
Note: The PR description mentions running 8. ✅ Documentation (EXCELLENT)Files reviewed:
Verdict: README correctly shows the new signature. 📊 SonarQube Duplication AnalysisSonarQube flagged 5.6% duplication (threshold is 3%). Let me analyze: Likely causes:
Recommendation: This duplication is acceptable because:
You could suppress this SonarQube warning for generated files by updating 🎯 Specific Issues FoundNONE - No critical or major issues foundMinor observations:
✨ Code Quality HighlightsWhat's done exceptionally well:
📝 RecommendationsMinor Suggestions (Optional):
✅ Final VerdictThis PR is production-ready.
The SonarQube duplication warning is a false positive due to generated code and expected wrapper patterns - not a real quality issue. Recommendation: APPROVE and MERGE (after running |
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>
This PR implements the change discussed in #95 to require a single TTL for all items in
put_many()operations.Summary
This breaking change simplifies the
put_manyAPI by requiring a single TTL value for all items in a batch operation, instead of supporting per-item TTLs. This aligns with industry standards (Django, Flask, redis-py) and addresses backend store limitations.Changes
put_many()IncorrectTTLCountErrorexceptionBreaking Change
Users who were passing a sequence of TTLs to
put_many()will need to:put_many()calls grouped by TTL value, orput()callsThis follows the same pattern used in Django and other major frameworks.
Post-Merge Action Required
After merging, run
make codegento regenerate the sync library.Fixes #95
🤖 Generated with Claude Code) | View job run | [Branch](https://github.com/strawgate/py-key-value/tree/claude/issue-95-20251026-0453