Skip to content

[BUG] Correct the Python package documentation - #683

Open
Seth Fitzsimmons (sethfitz) wants to merge 5 commits into
mainfrom
docs-accuracy-fixes
Open

[BUG] Correct the Python package documentation#683
Seth Fitzsimmons (sethfitz) wants to merge 5 commits into
mainfrom
docs-accuracy-fixes

Conversation

@sethfitz

Copy link
Copy Markdown
Collaborator

Summary

Fixes the documentation inaccuracies catalogued in #604 and #668, the two CLI defects that produced some of them, and adds the tag that one of them described but nothing emitted. Adds a test that keeps the whole class from recurring.

Two things want a decision rather than a review, both marked below: what the new overture tag should mean, and whether a top-level tests/ is the right home for a repo-wide check.

Every claim below was verified by running the code, not by reading it.

Documentation

packages/overture-schema/README.md documented from overture.schema import Building, Place and from overture.schema import parse, discover_models, json_schema. None of it resolves: overture.schema is a namespace root shipping only py.typed, and parse() exists nowhere in the codebase. The README predates #622, which moved the validation API out of the namespace root. Rewritten against the real surface — models from their theme packages, validate/validate_json from overture.schema.validation, discovery and json_schema from overture.schema.system.

Three READMEs claimed model_validate() accepts GeoJSON. It does not. Python mode reads the flat tabular shape; JSON mode reads GeoJSON; the two are exclusive, and the error a GeoJSON dict produces (theme Field required) does not point at the cause. Each now states the split. The round-trip examples also pass by_alias=True, without which class_ is emitted instead of class and the output will not re-validate.

packages/overture-schema-codegen/README.md described analyze_type() returning a TypeInfo with .kind/.base_type/.newtype_name, and imported TypeKind. Neither type exists; the signature is (FieldShape, bool, str | None). Replaced with a worked example whose output is transcribed from an actual run. The layer diagram and the package credited with discover_models are corrected too.

packages/overture-schema-pyspark/README.md used release 2026-06-17.0 in its S3 examples. The bucket retains only the current release, so that path 404s — aws s3 ls s3://overturemaps-us-west-2/release/ returns one prefix. Bumped, with a note on reading the current identifier from the STAC catalog's latest field, since any hardcoded release goes stale by design.

PYDANTIC_GUIDE.md prescribed a models.py / enums.py / types.py split inside a per-type subdirectory (#604). No theme package has such a file. Rewritten against the settled layout: one module per feature type at the theme root, _common.py for what a theme shares, a subpackage only for a type large enough to split. The JSON-Schema-container-to-mixin example now uses the real Named / Appearance mixins that Building actually inherits, and the container names on both sides (namesContainer, shapeContainer) exist in schema/.

The overture tag

overture-schema --help suggested --tag overture --tag feature for "official Overture types only", and discovery emitted no such tag, so the filter matched nothing (#668 item 4). Rather than delete the reference, overture-schema-common now ships an overture_provider that attaches the tag when any concrete arm of an entry point subclasses OvertureFeature; the tag is reserved to that package.

"Is this built on Overture's feature model?" is now answerable by tag, so a consumer asking it needs neither a dependency on overture-schema-common nor an issubclass call.

The help example is also corrected on a second axis: --tag is OR, so pairing overture with feature would have widened the scope rather than narrowed it. --tag overture alone is the filter that means what the help said.

Worth discussing before this merges: what should overture mean?

The help previously read "Official Overture types only", and that wording is wrong — it has been changed to "Only types built on the Overture feature model". The distinction matters, and people may reasonably expect either.

As implemented, the tag attaches to any OvertureFeature subclass reachable from an overture.models entry point, including a third party's:

class AcmeParkingMeter(OvertureFeature[Literal["acme"], Literal["parking_meter"]]):
    pass
# -> tags: {"feature", "overture", "overture:theme=acme"}

So it means "carries the Overture feature contract" — theme, type, id, version, sources, geometry — as distinct from feature, which says only that something is a Feature. It is not a claim of membership in the Overture schema. Reserving the tag to overture-schema-common governs who may emit it, not which models receive it; the provider that emits it does so for everyone.

If the intent is the narrower "types Overture publishes", that is a different predicate — provenance rather than base class — and would need something the base class does not carry: a distributor allowlist, a separate overture:approved tag, or a reserved theme set. Worth settling now, because the tag is public surface as soon as this merges and the narrow reading is not a superset of the broad one.

Note also overture:theme=acme in the example above: the theme tag has the same property today, and predates this PR.

CLI defects

--help rendered a literal \b. The command docstrings were raw strings, so Click's no-rewrap marker stayed two characters and every example block collapsed into one paragraph. Dropped the r prefix. pydocstyle's D301 pushes the other way and is a false positive for Click docstrings, so it is waived with the reason recorded once at the module level.

overture-codegen list printed a type expression instead of a name. The listing fell back to str(model) for anything without __name__, so Segment emitted several hundred characters of discriminator internals. It now reads the name off the entry point, which every registered model has.

Keeping it fixed

tests/test_documented_imports.py parses every fenced Python block in every tracked Markdown file and imports each overture.* statement it finds — 152 today, across 100+ files. A broken one fails the suite instead of accumulating.

The detector went through four review rounds, and the findings were mostly against the detector rather than the docs. Worth knowing what it does and does not do:

  • Block extraction and import parsing are separate functions, each unit-tested against inline fixtures. The corpus cannot test either: a form the matcher misses simply yields nothing, and a sweep that collects less still passes. Two rounds found exactly that failure — a control that passed with the branch it named deleted, and an indent fix that was silently revertible.
  • Blocks indented inside list items are matched and dedented. Without the dedent an IndentationError is a SyntaxError, which would file the block as "expected unparseable" and hide it twice.
  • REPL transcripts are unwrapped rather than skipped.
  • Blocks that are deliberately not valid Python are pinned by a digest of their body, not by count or index — a count stays put when one block breaks as another is fixed. An excused block may not contain an overture import, so the waiver cannot swallow the API citation it is excusing. It had, once.

make lint-only also now checks Python embedded in Markdown, with make format as the counterpart. That needs ruff 0.16, so the dependency floor moves from 0.13 — at the old floor the check finds no files and passes silently, and one CI job resolves at lowest-direct.

Worth discussing before this merges: a top-level tests/

This adds the repo's first top-level tests/ directory, which is a convention change worth a decision rather than a reviewer's shrug.

The test admits on two conditions, both recorded in tests/README.md so the directory does not become a catch-all:

  • Its subject spans packages, or sits outside them. It reads every tracked Markdown file, including root-level ones (PYDANTIC_GUIDE.md, GLOSSARY.md, CONTRIBUTING.md) that ship in no package.
  • It cannot run from one package's install. It imports across every package, overture.schema.codegen among them.

It started in packages/overture-schema/tests/, and that was worse in a specific way: it made that package's suite import a package its pyproject.toml does not declare, passing only because the workspace installs everything. Any package-scoped home has that problem.

The cost, plainly. pytest packages/ is no longer complete. Three Makefile targets and the mypy files list had to learn about tests/, and a contributor running pytest packages/ from habit misses it. make check does cover it — through ruff, ruff format, mypy and pytest.

Prior art, and what it argues. The repo already has repo-wide process code in Python: .github/workflows/scripts/package_versions.py and two action scripts. The convention there is to confine it to .github/, which is a real argument against a new top-level directory.

It also shows that convention's cost. Those three files sit outside every gate — [tool.ruff] src, mypy's files, and the pytest paths all point at packages/. That is not hypothetical: running the repo's own ruff config over them today reports two lint errors and two files needing reformatting, purely because nothing has ever applied it. Filed separately as #682; not fixed here, since it is unrelated to the documentation work.

So the choice is between a directory that is new but gated, and a convention that is established but has a hole in it.

The alternative, if a new top-level directory is unwelcome: a non-shipping workspace package (packages/overture-schema-repo-checks/ or similar) that declares dependencies on everything. That keeps pytest packages/ complete, at the cost of the repo's only package that publishes nothing — a pyproject.toml, a version, a changelog.d/, and an explicit exclusion from the publish workflow. I think that is more ceremony than the problem earns, but it is a one-commit change if the group disagrees.

Closes #604
Closes #668

Discovery emitted `feature` and `overture:theme=*` but never the plain
`overture` tag, so the filter the CLI's own help suggested matched nothing.

Add an `overture_provider` in overture-schema-common that attaches the tag
when any concrete arm of an entry point subclasses `OvertureFeature`, and
reserve the tag to that package. "Is this built on Overture's feature
model?" is now answerable by tag, so a consumer asking it needs neither a
dependency on this package nor an `issubclass` call.

The tag is not a claim that the type belongs to the Overture schema. Any
package can subclass `OvertureFeature` and register an entry point, and
this provider tags it like any other; reserving the tag governs who may
emit it, not which models receive it.

Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
The command docstrings were raw strings, so Click's `\b` no-rewrap marker
stayed two literal characters and every example block collapsed into one
paragraph. Drop the `r` prefix; pydocstyle's D301 pushes the other way and
is a false positive for Click docstrings, so it is waived with the reason
recorded once at the module level.

The "official Overture types only" examples suggested `--tag overture
--tag feature`, wrong twice over: the plain `overture` tag did not exist,
and `--tag` is OR, so pairing it with `feature` widens the scope instead
of narrowing it. Cite `--tag overture` alone, reworded -- the tag says a
type is built on Overture's feature model, not that it is official. Added
an example showing what the OR is for, and tag examples to `list-types`,
which had none.

The `--tag` syntax note illustrated namespaced tags with
`overture:approved`, which no provider can register. No shipped model emits
a bare namespaced tag, so the note describes the form instead.

`test_help_cites_only_tags_that_exist` covers the whole surface: it splits
rendered help per option and checks both citation forms -- an argument
(`--tag X`) and an illustration (`(e.g. X)`) -- against the set that
option accepts, since `--group-by` takes the key half of a `key=value` tag
and `--tag` does not. Judged against a global union instead, a phantom
passes in whichever option it was not written for.

Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
The listing fell back to `str(model)` for anything without `__name__`, so
`Segment` printed its whole `typing.Annotated[...]` type expression --
several hundred characters of discriminator internals in place of a name.

Read the name off the entry point instead. Every registered model has one,
class or union alias alike, and it is the name the rest of the toolchain
already uses.

Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
Fixes the inaccuracies catalogued in #604 and #668.

`packages/overture-schema/README.md` documented `from overture.schema
import Building, parse, discover_models, json_schema`. None of it
resolves: `overture.schema` is a namespace root shipping only `py.typed`
since #622 moved the validation API out of it, and `parse()` exists
nowhere. Rewritten against the real surface -- models from their theme
packages, `validate`/`validate_json` from `overture.schema.validation`,
discovery and `json_schema` from `overture.schema.system`.

Three READMEs claimed `model_validate()` accepts GeoJSON. It does not, and
the framing above it was also wrong: Overture publishes one shape, flat
and tabular. The models additionally read and write GeoJSON so the schema
works with tools that expect features rather than rows, and because that
is the representation the generated JSON Schema describes. The two modes
are not interchangeable, and the error a GeoJSON dict produces ("theme
Field required") does not point at the cause. Round-trip examples now pass
`by_alias=True`, without which `class_` is emitted and the output will not
re-validate.

The codegen README described `analyze_type()` returning a `TypeInfo` with
`.kind`/`.base_type` and imported `TypeKind`. Neither exists; it returns a
`(FieldShape, bool, str | None)` tuple. Replaced with a worked example
whose output is transcribed from a run, and corrected the layer diagram
and the package credited with `discover_models`.

The pyspark README's S3 examples named release 2026-06-17.0, which the
bucket no longer holds. Bumped, with a note on reading the current
identifier from the STAC catalog, since a hardcoded release goes stale by
design.

PYDANTIC_GUIDE.md prescribed a `models.py` / `enums.py` / `types.py` split
inside a per-type subdirectory (#604). No theme package has such a file.
Rewritten against the settled layout: one module per feature type at the
theme root, `_common.py` for what a theme shares, a subpackage only for a
type large enough to split. The container-to-mixin example now uses the
real `Named` / `Appearance` mixins `Building` inherits.

It also named two enum members that do not exist -- `Relationship` has
`COMPOSITION`, `AGGREGATION`, `HIERARCHY`, `ASSOCIATION`, and
`CONNECTS_TO`/`BELONGS_TO` are *roles*. A third instance was in
overture-schema-system's README. And its `$defs` migration example placed
an `Address` with `freeform`/`locality` in the addresses theme, claiming
`Building` carries one; that model is `places.Address`, and `Place` is
what holds it.

Removed the Reference section's five "Complete Templates": they prescribed
the dead file-name convention, and their content survives in Quick Start,
Quick Reference, and the Relationship Patterns section.

Python embedded in Markdown is now formatted with `ruff format`, so
documented snippets match the style of the code they describe. That needs
ruff 0.16, so the dependency floor moves from 0.13.

Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
Documentation drifts silently: a module moves, a helper is renamed, and the
README keeps confidently describing the old surface. #489, #604 and #668
are all instances. This makes the class fail the suite instead of
accumulating.

`tests/test_documented_imports.py` parses every fenced Python block in
every tracked Markdown file and checks two things: that each `overture.*`
import resolves, and that each `Enum.MEMBER` named exists on the enum. The
second is not redundant -- `from ... import Relationship` resolves whether
or not `Relationship.CONNECTS_TO` does.

Executing the blocks outright is not viable: most fail on undefined names,
because a documentation fragment legitimately omits its imports and uses
placeholder names the prose supplies. Imports and enum members are the
granularity that separates a defect from a fragment.

Block extraction and import parsing are separate, unit-tested functions.
The corpus cannot test either: a form the matcher misses simply yields
nothing, and a sweep that collects less still passes -- so the fixtures are
inline rather than drawn from repo content. Blocks indented inside list
items are dedented before parsing, since an `IndentationError` is a
`SyntaxError` and would otherwise be filed as "expected unparseable".
Blocks that are deliberately not valid Python are pinned by a digest of
their body rather than a count, which would stay put when one breaks as
another is fixed; and an excused block may not contain an `overture`
import, so the waiver cannot swallow the API citation it excuses.

The tests live outside `packages/` because their subject is the repo's
Markdown, including root-level files belonging to no package, and they
import across every package -- `overture.schema.codegen` among them, which
no single distribution depends on. `tests/README.md` records the admission
rule. `make check` reaches the tree through ruff, `ruff format`, mypy and
pytest; a bare `pytest packages/` does not.

`lint-only` also checks Python embedded in Markdown, with `make format` as
the counterpart. At the old ruff floor that check silently finds no files,
and CI resolves one job at `lowest-direct`.

Signed-off-by: Seth Fitzsimmons <seth@mojodna.net>
@github-actions

Copy link
Copy Markdown

🗺️ Schema reference docs preview is live!

🌍 Preview https://staging.overturemaps.org/schema/pr/683/schema/index.html
🕐 Updated Aug 19, 2026 20:20 UTC
📝 Commit ae0d174
🔧 env SCHEMA_PREVIEW true

Note

♻️ This preview updates automatically with each push to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change type - minor 🤏 Minor schema change. See https://lf-overturemaps.atlassian.net/wiki/x/GgDa

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python package documentation is inaccurate in several places [DOCS] PYDANTIC_GUIDE.md documents a package layout the repo no longer uses

1 participant