[BUG] Divisions: allow both is_land and is_territorial to be true#546
Open
Alex Iannicelli (atiannicelli) wants to merge 11 commits into
Open
[BUG] Divisions: allow both is_land and is_territorial to be true#546Alex Iannicelli (atiannicelli) wants to merge 11 commits into
Alex Iannicelli (atiannicelli) wants to merge 11 commits into
Conversation
Replace @radio_group (exactly one must be true) with a new @require_any_true constraint (at least one must be true) on DivisionArea and DivisionBoundary models. Changes: - Add require_any_true constraint to overture-schema-system - Update division_area.py and division_boundary.py to use it - Change division_boundary.yaml schema from oneOf to anyOf - Remove bad-not-both counterexample (now valid) - Add both_land_territorial positive examples - Update codegen model_constraints.py for new constraint type - Regenerate baseline schemas Closes #542 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
🗺️ Schema reference docs preview is live!
Note ♻️ This preview updates automatically with each push to this PR. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Copilot started reviewing on behalf of
Alex Iannicelli (atiannicelli)
June 18, 2026 21:05
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the divisions schema to allow is_land and is_territorial to both be true by replacing the “exactly one true” constraint (@radio_group / oneOf) with an “at least one true” constraint (@require_any_true / anyOf) across the Pydantic models and the hand-written YAML schema.
Changes:
- Add a new
@require_any_truemodel constraint inoverture-schema-systemand export it. - Update
DivisionArea/DivisionBoundaryto use@require_any_true("is_land", "is_territorial"). - Update the YAML + baseline JSON schemas and examples/counterexamples to reflect
anyOfbehavior and allow both flags to betrue.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/divisions/division_boundary.yaml | Switches oneOf → anyOf for land/territorial flags in the YAML schema. |
| packages/overture-schema-system/src/overture/schema/system/model_constraint/require_any_true.py | Introduces the new model constraint and JSON Schema generation hook. |
| packages/overture-schema-system/src/overture/schema/system/model_constraint/init.py | Exports require_any_true / RequireAnyTrueConstraint. |
| packages/overture-schema-divisions-theme/src/overture/schema/divisions/division_boundary.py | Replaces @radio_group with @require_any_true. |
| packages/overture-schema-divisions-theme/src/overture/schema/divisions/division_area.py | Replaces @radio_group with @require_any_true. |
| packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_constraints.py | Adds description + field-affecting logic for the new constraint type. |
| packages/overture-schema-divisions-theme/tests/division_boundary_baseline_schema.json | Updates generated baseline schema to use anyOf. |
| packages/overture-schema-divisions-theme/tests/division_area_baseline_schema.json | Updates generated baseline schema to use anyOf. |
| examples/divisions/division_boundary/both_land_territorial.yaml | Makes the “both true” example valid (removes expected-error, adjusts fields). |
| examples/divisions/division_area/both_land_territorial.yaml | Adds a new valid “both true” example for division areas. |
| counterexamples/divisions/division_boundary/bad-not-both.yaml | Removes a counterexample that is now valid under anyOf. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
becab7a to
3df9e21
Compare
Victor Schappert (vcschapp)
left a comment
Collaborator
There was a problem hiding this comment.
Two comments:
- It might be useful to generalize
require_any_true. - The Python code doesn't use
examples/andcounterexamples/it usesreference/examples/andreference/counterexamples/- apologies for this inconsistency but could these be updated also?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alex Iannicelli <atiannicelli@gmail.com>
108710d to
3b64d0a
Compare
John McCall (lowlydba)
approved these changes
Jul 2, 2026
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.
Summary
Replaces the
@radio_groupconstraint (exactly one must be true) with a new@require_any_trueconstraint (at least one must be true) onDivisionAreaandDivisionBoundarymodels. This allows bothis_landandis_territorialto betruesimultaneously.Changes
@require_any_trueconstraint inoverture-schema-system— validates that at least one field in a group ofboolfields isTrue, generatinganyOfin JSON Schema (vsoneOffrom@radio_group)DivisionAreaandDivisionBoundaryto use@require_any_true("is_land", "is_territorial")division_boundary.yaml) fromoneOf→anyOfbad-not-bothcounterexamples (now valid) and addedboth_land_territorialpositive examplesmodel_constraints.pyto handle the new constraint typeCloses #542