[ENHANCEMENT](schema) Remove the non-null defaults from the schema models - #697
Open
Seth Fitzsimmons (sethfitz) wants to merge 2 commits into
Open
[ENHANCEMENT](schema) Remove the non-null defaults from the schema models#697Seth Fitzsimmons (sethfitz) wants to merge 2 commits into
Seth Fitzsimmons (sethfitz) wants to merge 2 commits into
Conversation
Implements the policy in #695 for three of the four remaining sites: no schema field carries a non-null default, and where a value would come from a default the publisher supplies it in the published data. - `Stacked.level` drops `= 0` and becomes `None`. The absence sentence goes on the field rather than the `Level` NewType, because `Level` is also the type of the required `LevelRule.value`, where absence is impossible. Removing the default also retires the `# type: ignore[assignment]` that existed only to admit it. - `Land.class_`, `Land.subtype`, `Water.class_`, `Water.subtype` become required, matching `Infrastructure` and `LandUse`. No consumer sees a difference. Measured on release 2026-08-19.0, one partition per type: `level = 0` appears in 0 of 1,174,629 land and 0 of 1,985,644 water rows, and `class`/`subtype` are never null in either. Both partitions validate clean against the changed models. `SpeedLimitRule.is_max_speed_variable` is deliberately left out. Its default cannot be removed without stating what absence means, and the data does not settle that: the publisher writes an explicit `false` for closed-world booleans (`building.has_parts` is 0 null / 4,777,998 false) but never for this one (0 false in 1,376,379 rules across three partitions), so absence is an unrecorded value rather than a recorded "no". That call needs a domain owner. Refs #696 Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
Completes the removal begun in the previous commit: `SpeedLimitRule. is_max_speed_variable` drops `= False`. A rule parsed without the flag now yields `None` instead of `False`. The description is left as it stands. What absence means here is a domain question the schema should not answer by inference, and the existing wording does not define the term well enough to extend safely. Both are worth settling, separately, with the transportation folks. Refs #696 Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
🗺️ Schema reference docs preview is live!
Note ♻️ This preview updates automatically with each push to this PR. |
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.
Closes #696
Implements the policy adopted in #695 for the four remaining declaration sites. No schema field carries a non-null default after this change; a census over
discover_models()reports zero, across 54 models.Eleven fields, four sites in four files — the other seven are inherited and re-reported by subclasses.
common/level.pyStacked.level= 0Level | None = Nonebase/land.pyLand.class_,Land.subtypeLandClass.LAND,LandSubtype.LANDbase/water.pyWater.class_,Water.subtypeWaterClass.WATER,WaterSubtype.WATERtransportation/segment/_common.pySpeedLimitRule.is_max_speed_variable= Falsebool | None, no defaultLandandWaternow matchInfrastructureandLandUse, which already declaredclass_andsubtyperequired with no default. Removing theleveldefault also retires a# type: ignore[assignment]that existed only to admit it —Levelis aNewTypeoverAnnotated[int32, ...], so0was never a value of the field's declared type.The absence sentence went on the field, not the type
Stacked.level's description states that a feature without a level is at visual level. That sentence is on the field annotation rather than on theLevelNewType, becauseLevelis also the type ofLevelRule.valueintransportation/segment/_common.py, which is required — an absence sentence there renders into a context where absence is impossible.is_max_speed_variablekeeps its descriptionThe default is removed; the description is untouched. What absence means for this field is a domain question, and the existing wording ("Indicates a variable speed corridor") does not define the term precisely enough to extend safely. Both are worth settling with the transportation folks, separately from this change. Flagging it explicitly because #696's second acceptance criterion — that every meaning a removed default carried is stated in the field description — is met for
leveland moot forclass_/subtype, but left open here.No consumer sees a difference
Measured against release
2026-08-19.0, one partition per type. No published row carries any of these defaults:Land.levelWater.levelLand.class/subtypeWater.class/subtypeSpeedLimitRule.is_max_speed_variablefalse)The publisher already writes
classandsubtypeon every row, so making them required tightens the model to what the release already contains.Verification
LevelNewTypeproblem above.land,waterandsegmentpartitions from2026-08-19.0validate clean underoverture-validate --skip-schema-check: 0 errors across 5,816,361 rows. That zero is a real result, not a dead check — injecting nulls into 333classand 200subtypevalues makes the same command reportclass:required/subtype:requiredand exit 1._class_checksplits into_class_required_check+_class_enum_check, same forsubtype); there is noStructTypediff, sinceschema_builder.pyrenders every field nullable regardless of requiredness. Markdown drops(optional)from the four fields and the example rows change0→nullandfalse→null— those examples were displaying the injected default rather than what the release contains.Follow-ups
is_max_speed_variableasserts, and what its absence means.overture-schema-systemcan run it too.