feat: migrate church_sui_iuris to CESIDR esi: IDs - #9
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe schema now uses CESIDR ChangesCESIDR cross-reference adoption
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/generate_seed.py`:
- Around line 100-104: The church_sui_iuris validation in the entry-checking
block must reject the bare "esi:" value. Replace the assert-based prefix check
with an explicit validation that requires at least one character after "esi:"
and aborts before generated output is written when invalid references are found.
🪄 Autofix
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 Plus
Run ID: 577fec66-792f-4ce7-a300-1dc489f94fed
📒 Files selected for processing (4)
README.mddata/circumscriptions.jsondocs/schema-proposal.mdscripts/generate_seed.py
|
Verified against the code. One of the three points holds; fixed in Fixed — bare
|
| value | exit | output unchanged |
|---|---|---|
"esi:" |
1 | ✅ |
"latin" |
1 | ✅ |
"esi:Latin" |
1 | ✅ |
"esi:-latin" |
1 | ✅ |
Worth noting it was latent, not live: church_sui_iuris is set in exactly one place and no MANUAL override touches the field, so no entry could reach that state today. The check guards the field as it grows, and it should do that correctly.
Skipped — "aborts before generated output is written"
Already true. The assertion is at line 104; out is constructed at 105 and written at 115–118. Nothing is written when validation fails — confirmed by the byte-identical column above, which was the point of measuring it.
Skipped — "replace the assert-based prefix check"
The real concern here is that assert is stripped under python -O. That is legitimate, but it applies identically to the duplicate-ID check (line 93) and the ctype: resolution check (line 99). Converting one of the three in isolation would leave the file inconsistent without closing the gap.
If the -O concern should be addressed, the right change is all three together, in its own PR — happy to open one. Out of scope for a migration that adds a prefix.
Resolves open question 6: `church_sui_iuris` now carries the full cross-reference (`"church_sui_iuris": "esi:latin"`) rather than a bare slug, matching how `type` carries `ctype:` and how the family carries `rp:` and `mr:` references. The CESIDR slugs were minted to match the values this field already held — latin, ukrainian, maronite, syro-malabar — so the migration adds a prefix and nothing else. No value was renamed, no entry changed meaning, and all 2,935 entries move together from `latin` to `esi:latin`. Rule 4 gains a clarification the migration makes necessary: the slug qualifier for an eparchy sharing a city with a Latin see stays bare (`circ:us-philadelphia-ukrainian`, not `-esi-ukrainian`), because that is part of an identifier rather than a reference to one. generate_seed.py asserts the `esi:` prefix. It cannot resolve these values — CESIDR is a separate repository — so this is a shape check only, and cross-repository resolution is left to the committee alongside the prefix decision. Based on the circumscription-types branch; that PR should merge first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`startswith("esi:")` accepted the bare string `"esi:"`, which carries no
slug and resolves to nothing. Matching the full shape instead also rejects
uppercase and leading/trailing hyphens.
Latent rather than live: the value is set in one place and no MANUAL
override touches the field, so no entry could reach the bad state today. The
check exists to guard the field as it grows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ab563e6 to
dd9d9ac
Compare
Resolves open question 6.
church_sui_iurisnow carries the full cross-reference rather than a bare slug:{ "id": "circ:us-boston", "church_sui_iuris": "esi:latin", "type": "ctype:archdiocese" }Both attributes are now
<registry>:<slug>references, matching how the family already works — COECDR embeds CRPDRrp:IDs, CDOCTDR embeds CRMEDRmr:IDs.A rename-free migration
CESIDR slugs were minted specifically to match the values this field already carried —
latin,ukrainian,maronite,syro-malabar, exactly as rule 4 listed them. So this adds a prefix and nothing else. No value renamed, no entry changed meaning; all 2,935 entries move together fromlatintoesi:latin.Rule 4 clarification
The migration forces a distinction worth stating explicitly. Where an eparchy shares a city with a Latin see, the slug qualifier stays bare:
That qualifier is part of an identifier, not a reference to one. Only the
church_sui_iurisfield takes the prefix.Enforcement, and its limit
generate_seed.pyasserts theesi:prefix — verified by reverting a value, which exits 1 withchurch_sui_iuris values missing the esi: prefix: ['latin'].It is a shape check only. CESIDR is a separate repository, so the generator cannot resolve these IDs the way it resolves
ctype:values against the local types registry. Cross-repository resolution — whether by submodule, published index, or convention — is a real question and belongs with the namespace decision in open question 1.Merge order
Based on
feat/circumscription-types(#8), and targeted at that branch rather thanmain, because both touchschema-proposal.md,README.mdandgenerate_seed.py. Merge #8 first; this then retargets tomaincleanly. Reviewing the two-commit diff shows only this change.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
esi:-prefixed church identifiers.Data Validation