Release 0.2.1: reject booleans, and stop overstating the retries - #5
Merged
Conversation
bool subclasses int, so True reached the integer branch of _normalize_source and returned the reference belonging to 1, and False the one belonging to 0. A caller who passed a flag where an identifier was meant got a real, valid-looking reference that belonged to a different record. Both raise TypeError now. A type checker will not catch that call, and cannot: bool subclasses int in the type system exactly as it does at runtime, so SourceIdentifier admits True and mypy is content. Python has no way to spell "int but not bool". The guard is therefore a runtime one, the docstring says so, and the tests carry no suppression on those calls because none is possible. expected_collisions() documented itself as an error rate -- "the number of writes per bucket a caller should expect to retry". It is not, and the gap is not academic. It returns expected colliding *pairs*, and a suffix drawn k times is k * (k - 1) / 2 pairs but only k - 1 rejected inserts, so the two agree while a bucket is sparse and part company once it fills. Simulated: two thousand references over three digits is 1999 pairs and about 1135 rejected inserts, so the old wording overstated the retries by more than half, exactly where a crowded caller would be reading it. The behaviour was always right; only the claim about it was wrong. The docstring and README now say it counts pairs, warn against sizing a retry budget with it, and a test pins the divergence so the claim cannot quietly return. The function people actually want -- the expected number of rejected inserts, n - space * (1 - (1 - 1/space)**n), which tracks the simulation exactly -- is additive, and belongs in 0.3.0 rather than a patch. References produced by 0.2.0 are unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change rejects boolean reference sources, clarifies ChangesInput validation and collision semantics
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bool subclasses int, so True reached the integer branch of _normalize_source and returned the reference belonging to 1, and False the one belonging to 0. A caller who passed a flag where an identifier was meant got a real, valid-looking reference that belonged to a different record. Both raise TypeError now.
A type checker will not catch that call, and cannot: bool subclasses int in the type system exactly as it does at runtime, so SourceIdentifier admits True and mypy is content. Python has no way to spell "int but not bool". The guard is therefore a runtime one, the docstring says so, and the tests carry no suppression on those calls because none is possible.
expected_collisions() documented itself as an error rate -- "the number of writes per bucket a caller should expect to retry". It is not, and the gap is not academic. It returns expected colliding pairs, and a suffix drawn k times is k * (k - 1) / 2 pairs but only k - 1 rejected inserts, so the two agree while a bucket is sparse and part company once it fills. Simulated: two thousand references over three digits is 1999 pairs and about 1135 rejected inserts, so the old wording overstated the retries by more than half, exactly where a crowded caller would be reading it.
The behaviour was always right; only the claim about it was wrong. The docstring and README now say it counts pairs, warn against sizing a retry budget with it, and a test pins the divergence so the claim cannot quietly return. The function people actually want -- the expected number of rejected inserts, n - space * (1 - (1 - 1/space)**n), which tracks the simulation exactly -- is additive, and belongs in 0.3.0 rather than a patch.
References produced by 0.2.0 are unchanged.
Summary by CodeRabbit
Bug Fixes
Documentation
Release