Release 1.0.0: lock the API - #9
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughVersion 1.0.0 centralizes configuration validation, adds structured parsing, changes verification to use parsing, removes legacy APIs, updates public exports, expands contract tests and documentation, and broadens CI across operating systems and Python versions. ChangesAPI stabilization
Cross-platform CI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CompactRef
participant ParsedReference
Caller->>CompactRef: parse(reference)
CompactRef-->>ParsedReference: structured reference parts
Caller->>CompactRef: verify(reference)
CompactRef->>CompactRef: parse(reference)
CompactRef-->>Caller: verification result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
neosergio
force-pushed
the
feat/v1.0.0
branch
2 times, most recently
from
July 14, 2026 12:07
c2c9461 to
21e9121
Compare
There was a problem hiding this comment.
Pull request overview
This PR finalizes the 1.0.0 release by locking CompactRef’s public API and output determinism guarantees, tightening reference verification semantics, and pinning behavior with stronger cross-platform tests and documentation checks.
Changes:
- Replace separator-splitting verification with positional parsing (
CompactRef.parse()) and strictverify()behavior; remove the unsafeverify_reference()free function. - Add runtime configuration validation (including “int but not bool” enforcement) and stricter “checked schemes must be checkable” rules (fixed-width/portable/spellable dates; separator not in alphabet).
- Add golden tests + README/doc consistency checks; expand CI to run tests across OSes and Python versions.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_properties.py | Expands property testing to cover single-char edits, stray separators, and checkability constraints. |
| tests/test_integration.py | Updates SQLAlchemy integration example to use reference_length and adjusts exhaustion test for checked schemes. |
| tests/test_golden.py | Adds golden-string contract tests pinning outputs, API surface (__all__), defaults, and time-bucketing behavior. |
| tests/test_docs.py | Adds tests that README only references real, supported API names and doesn’t “use” removed functions. |
| tests/test_core.py | Updates tests for removed APIs, new strict verification/parse behavior, and runtime type/value enforcement. |
| src/compactref/reference.py | Introduces parse(), ParsedReference, InvalidReferenceError, and rewrites verify() to be positional/structural + checksum. |
| src/compactref/core.py | Adds portable strftime wrapper, checkability validation, and centralized runtime configuration validation; removes deprecated/unsafe functions. |
| src/compactref/init.py | Updates exports to include new public types/exceptions and bumps version to 1.0.0. |
| ROADMAP.md | Updates roadmap to reflect the 1.0 lock and moves future work into additive-only sections. |
| README.md | Refreshes examples, documents new parsing/verification model, and pins outputs to real library behavior. |
| pyproject.toml | Bumps version/classifier and adds Windows tzdata handling via extras/dev deps. |
| CHANGELOG.md | Adds detailed 1.0.0 release notes documenting the locked contract and behavioral fixes. |
| .github/workflows/ci.yml | Expands CI matrix across OSes/Python versions and adds pip check to enforce no accidental deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/compactref/core.py (2)
100-128: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCombine the two
_PROBE_DATESpasses.
widthsandrenderedeach iterate all 54 probe dates through_render_date(up to 108strftimecalls total). A single pass can compute both the rendered string and its characters, halving the work — and this function is called on every checkedgenerate_reference()/CompactRefbuild (see comment below on the call-site cost).♻️ Proposed refactor
- widths = { - len(_render_date(moment, date_format)) for moment in _PROBE_DATES - } - if len(widths) > 1: + rendered_by_moment = [ + _render_date(moment, date_format) for moment in _PROBE_DATES + ] + widths = {len(rendered) for rendered in rendered_by_moment} + if len(widths) > 1: raise ValueError( f"a checked reference needs a date of a fixed width, but " f"{date_format!r} renders to widths {sorted(widths)}. Verifying " f"reads a reference by position, so a width that moves would " f"make references verify for part of the year and be rejected " f"for the rest. Pad the field ('%d', not '%-d') or use a fixed " f"one ('%m', not '%B')." ) - rendered = { - character - for moment in _PROBE_DATES - for character in _render_date(moment, date_format) - } - unrepresentable = sorted(rendered - set(alphabet)) + rendered_characters = {c for r in rendered_by_moment for c in r} + unrepresentable = sorted(rendered_characters - set(alphabet))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compactref/core.py` around lines 100 - 128, Combine the width and character validation passes over _PROBE_DATES into one loop, rendering each moment once with _render_date and accumulating both rendered-string lengths and characters. Preserve the existing width and unrepresentable-character validation behavior and error messages in the surrounding validation logic.
315-346: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip revalidating
CompactRef’s frozen config on everygenerate()call
generate_reference()still needs its own checks, butCompactRef.generate()forwards a config that was already validated at construction. Re-running_validate_configuration()and, forcheck=True,_validate_checkable()on every call adds avoidable hot-path overhead; the checked path repeats 108strftime()calls each time. Add an internal fast path or cache the validator results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compactref/core.py` around lines 315 - 346, The CompactRef.generate path currently revalidates its frozen configuration on every call, including the expensive _validate_checkable strftime work. Update CompactRef.generate and its configuration handling to reuse construction-time validation results or take an internal validated fast path, while preserving generate_reference’s independent validation and all existing input checks for per-call values such as generated_at, source, and namespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 70-71: Replace the pip check step in the dependency validation
workflow with a direct inspection of the built package metadata, asserting that
its runtime Requires-Dist list is empty. Keep the existing intent and failure
behavior so any dependency declared in pyproject.toml causes CI to fail.
In `@src/compactref/reference.py`:
- Around line 377-386: Update the stray-character validation in the reference
parsing/checking flow to include the checked date field alongside suffix and
check_character when constructing body, while preserving existing behavior for
unchecked schemes. Add a regression test near the existing core tests covering a
checked CompactRef with date_format="0" rejecting a leading date character
outside the alphabet.
- Around line 136-150: Add a shared date-format validator that rejects
locale-dependent directives (%a, %A, %b, %B, %p, %c, %x, and %X) before
deterministic output is established. Invoke it from both CompactRef
initialization near _render_date and generate_reference, while preserving the
existing rendering and checkability validation behavior.
---
Nitpick comments:
In `@src/compactref/core.py`:
- Around line 100-128: Combine the width and character validation passes over
_PROBE_DATES into one loop, rendering each moment once with _render_date and
accumulating both rendered-string lengths and characters. Preserve the existing
width and unrepresentable-character validation behavior and error messages in
the surrounding validation logic.
- Around line 315-346: The CompactRef.generate path currently revalidates its
frozen configuration on every call, including the expensive _validate_checkable
strftime work. Update CompactRef.generate and its configuration handling to
reuse construction-time validation results or take an internal validated fast
path, while preserving generate_reference’s independent validation and all
existing input checks for per-call values such as generated_at, source, and
namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a272869-accf-403d-a23e-a00d69de84c5
📒 Files selected for processing (13)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mdROADMAP.mdpyproject.tomlsrc/compactref/__init__.pysrc/compactref/core.pysrc/compactref/reference.pytests/test_core.pytests/test_docs.pytests/test_golden.pytests/test_integration.pytests/test_properties.py
The promise: names, parameters, the default output format, the exception contract, and determinism. None of them break without a 2.0.0. A reference generated by 0.1.0 still resolves, byte for byte, and a test pins the strings to prove it -- every argument added across six releases defaults to doing nothing. Three ways a scheme could fail to verify its own references, all found in review, all now impossible to build. verify() pulled a reference apart on the separator and deleted every occurrence, so ORD--20260714-..., ORD-20260714-...- and ORD-20260714-047-1283 all came back True. That is exactly the confusion the check character exists to prevent: a caller could not tell a typo from a reference that does not exist. It reads by position now -- prefix matched literally, each field taken at its known width, each separator required where it belongs, nothing allowed to trail, and a character outside the alphabet refused rather than skipped. A separator drawn from the alphabet was eaten out of the suffix by that same strip. A date_format whose width moves -- %B is "January" one month and "December" another -- broke the length check, so references verified for part of the year and were rejected for the rest. Both fail at construction now. The width guard probes 54 datetimes, not the two suggested: 1 January and 31 December are both midnight, so an unpadded hour renders "0" in each and looks fixed when it is not. And a check character cannot cover what its alphabet cannot spell. Luhn skips any character it has no index for, so the hyphen in %Y%m%d-%H could be changed to anything undetected, and under an alphabet of letters the date was not protected at all. A checked scheme must now render its date in its own alphabet; an hourly bucket spells itself %Y%m%d%H and is covered like everything else. Reading by position is what makes the width guard load-bearing rather than decorative, and Hypothesis found the alphabet hole from the other side: under "01" the 2 of a 2026 date is skipped, and editing it to a 0 left the check unmoved. My property strategy could not have found the separator overlap -- it only ever drew "-" and "/", which appear in none of the alphabets it draws. A property test explores the space you hand it and no further. The strategy is widened; a single-character edit anywhere in a reference, and a stray separator anywhere in it, are invariants now. expected_collisions() is gone, deprecated since 0.3.0. It counted colliding pairs while its name promised a count of collisions to handle. verify_reference() is gone too, and this is the version that must remove it. Verifying a reference means knowing the alphabet, prefix, separator, length and date format it was generated with, and a free function has to be told all of that with no way to tell when it has been told wrong. It returned False for a valid reference whose configuration the caller mistyped -- which reads as "that is a typo", so you tell a customer their good reference is invalid. It could not see that a reference carried no check character either, and "verified" one about once in len(alphabet). Neither defect is visible from inside the function; only binding the configuration to the object fixes it. Deprecating it into 1.x was my first instinct and it was wrong: 1.0.0 is the last version allowed to remove anything, and a DeprecationWarning most projects never display would have left it quietly working for the whole of 1.x. The README's "Possible output" blocks have been fabricated since 0.1.0. They showed a suffix of 482731 for a ULID the library renders as 385177. Every literal in the file is now checked against what the library actually returns.
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
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.
The promise: names, parameters, the default output format, the exception contract, and determinism. None of them break without a 2.0.0. A reference generated by 0.1.0 still resolves, byte for byte, and a test pins the strings to prove it -- every argument added across six releases defaults to doing nothing.
Three ways a scheme could fail to verify its own references, all found in review, all now impossible to build.
verify() pulled a reference apart on the separator and deleted every occurrence, so ORD--20260714-..., ORD-20260714-...- and ORD-20260714-047-1283 all came back True. That is exactly the confusion the check character exists to prevent: a caller could not tell a typo from a reference that does not exist. It reads by position now -- prefix matched literally, each field taken at its known width, each separator required where it belongs, nothing allowed to trail, and a character outside the alphabet refused rather than skipped.
A separator drawn from the alphabet was eaten out of the suffix by that same strip. A date_format whose width moves -- %B is "January" one month and "December" another -- broke the length check, so references verified for part of the year and were rejected for the rest. Both fail at construction now. The width guard probes 54 datetimes, not the two suggested: 1 January and 31 December are both midnight, so an unpadded hour renders "0" in each and looks fixed when it is not.
And a check character cannot cover what its alphabet cannot spell. Luhn skips any character it has no index for, so the hyphen in %Y%m%d-%H could be changed to anything undetected, and under an alphabet of letters the date was not protected at all. A checked scheme must now render its date in its own alphabet; an hourly bucket spells itself %Y%m%d%H and is covered like everything else.
Reading by position is what makes the width guard load-bearing rather than decorative, and Hypothesis found the alphabet hole from the other side: under "01" the 2 of a 2026 date is skipped, and editing it to a 0 left the check unmoved. My property strategy could not have found the separator overlap -- it only ever drew "-" and "/", which appear in none of the alphabets it draws. A property test explores the space you hand it and no further. The strategy is widened; a single-character edit anywhere in a reference, and a stray separator anywhere in it, are invariants now.
expected_collisions() is gone, deprecated since 0.3.0. It counted colliding pairs while its name promised a count of collisions to handle.
verify_reference() is gone too, and this is the version that must remove it. Verifying a reference means knowing the alphabet, prefix, separator, length and date format it was generated with, and a free function has to be told all of that with no way to tell when it has been told wrong. It returned False for a valid reference whose configuration the caller mistyped -- which reads as "that is a typo", so you tell a customer their good reference is invalid. It could not see that a reference carried no check character either, and "verified" one about once in len(alphabet). Neither defect is visible from inside the function; only binding the configuration to the object fixes it. Deprecating it into 1.x was my first instinct and it was wrong: 1.0.0 is the last version allowed to remove anything, and a DeprecationWarning most projects never display would have left it quietly working for the whole of 1.x.
The README's "Possible output" blocks have been fabricated since 0.1.0. They showed a suffix of 482731 for a ULID the library renders as 385177. Every literal in the file is now checked against what the library actually returns.
Summary by CodeRabbit
New Features
CompactRef.parse(), including parsed reference components and clear invalid-reference errors.Changes
verify_reference()andexpected_collisions()APIs; migration guidance is included.Documentation