Skip to content

Commit 0cbf205

Browse files
mojodnavcschapp
authored andcommitted
refactor: replace runtime asserts with proper checks
Assert statements used for runtime validation disappear under python -O. Replace with TypeError/ValueError raises in validate_example, TypeIdentity.of, _SourceTypeIdentityMixin, _format_constraint, _linked_type_identity, type_collection. Guard _find_common_base against empty members list. Collapse duplicate datetime.datetime/date branches. Rename _format_field_list → _backtick_join (returns string, not list). Signed-off-by: Seth Fitzsimmons <sethfitz@amazon.com>
1 parent 4daa0e8 commit 0cbf205

File tree

8 files changed

+29
-21
lines changed

8 files changed

+29
-21
lines changed

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ test: uv-sync
1717
@uv run pytest -W error packages/ -x -q --tb=short
1818

1919
test-only:
20-
@uv run pytest -W error packages/ -x
20+
@uv run pytest -W error packages/ -x -q --tb=short
2121

2222
coverage: uv-sync
2323
@uv run pytest packages/ --cov overture.schema --cov-report=term --cov-report=html && open htmlcov/index.html

packages/overture-schema-codegen/src/overture/schema/codegen/extraction/examples.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,9 @@ def validate_example(
107107
known_keys = _known_field_keys(model_fields)
108108
preprocessed = _inject_literal_fields(model_fields, raw)
109109
preprocessed = _strip_null_unknown_fields(preprocessed, known_keys)
110-
result: BaseModel = TypeAdapter(validation_type).validate_python(preprocessed)
111-
assert isinstance(result, BaseModel)
110+
result: object = TypeAdapter(validation_type).validate_python(preprocessed)
111+
if not isinstance(result, BaseModel):
112+
raise TypeError(f"Expected BaseModel instance, got {type(result).__name__}")
112113
return result
113114

114115

packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_constraints.py

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ class _ConstraintEntry:
3434
field_names: frozenset[str]
3535

3636

37-
def _format_field_list(names: tuple[str, ...]) -> str:
37+
def _backtick_join(names: tuple[str, ...]) -> str:
3838
"""Format field names as backtick-quoted, comma-separated list."""
3939
return ", ".join(f"`{n}`" for n in names)
4040

@@ -73,7 +73,7 @@ def _describe_condition(condition: object) -> str:
7373

7474
def _describe_conditional(constraint: _ConditionalConstraint) -> str:
7575
"""Describe a require_if or forbid_if constraint."""
76-
fields = _format_field_list(constraint.field_names)
76+
fields = _backtick_join(constraint.field_names)
7777
verb = _conditional_verb(constraint)
7878
cond = _describe_condition(constraint.condition)
7979
return f"{fields} {_plural_verb(constraint.field_names)} {verb} when {cond}"
@@ -110,7 +110,7 @@ def _describe_consolidated(
110110
) -> str:
111111
"""Describe a group of consolidated conditional constraints."""
112112
first = constraints[0]
113-
fields = _format_field_list(first.field_names)
113+
fields = _backtick_join(first.field_names)
114114
verb = _conditional_verb(first)
115115
cond_field = _as_field_eq(first).field_name
116116
values = ", ".join(f"`{_as_field_eq(c).value}`" for c in constraints)
@@ -151,11 +151,9 @@ def _describe_one(constraint: ModelConstraint) -> str | None:
151151
if isinstance(constraint, NoExtraFieldsConstraint):
152152
return None
153153
if isinstance(constraint, RequireAnyOfConstraint):
154-
return (
155-
f"At least one of {_format_field_list(constraint.field_names)} must be set"
156-
)
154+
return f"At least one of {_backtick_join(constraint.field_names)} must be set"
157155
if isinstance(constraint, RadioGroupConstraint):
158-
return f"Exactly one of {_format_field_list(constraint.field_names)} must be `true`"
156+
return f"Exactly one of {_backtick_join(constraint.field_names)} must be `true`"
159157
if isinstance(constraint, MinFieldsSetConstraint):
160158
return f"At least {constraint.count} fields must be set"
161159
if isinstance(constraint, (RequireIfConstraint, ForbidIfConstraint)):

packages/overture-schema-codegen/src/overture/schema/codegen/extraction/specs.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,10 @@ class TypeIdentity:
4949
@classmethod
5050
def of(cls, obj: object) -> TypeIdentity:
5151
"""Derive a TypeIdentity from a named object (class, NewType, etc.)."""
52-
assert obj is not None
53-
return cls(obj, obj.__name__) # type: ignore[attr-defined]
52+
name = getattr(obj, "__name__", None)
53+
if name is None:
54+
raise TypeError(f"Cannot derive TypeIdentity from {obj!r}: no __name__")
55+
return cls(obj, name)
5456

5557
def __eq__(self, other: object) -> bool:
5658
return isinstance(other, TypeIdentity) and self.obj is other.obj
@@ -78,7 +80,8 @@ class _SourceTypeIdentityMixin:
7880

7981
@property
8082
def identity(self) -> TypeIdentity:
81-
assert self.source_type is not None
83+
if self.source_type is None:
84+
raise ValueError(f"Cannot derive identity for {self.name}: no source_type")
8285
return TypeIdentity(self.source_type, self.name)
8386

8487

packages/overture-schema-codegen/src/overture/schema/codegen/extraction/union_extraction.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
def _find_common_base(members: list[type[BaseModel]]) -> type[BaseModel]:
2020
"""Find the most-derived common BaseModel ancestor of all members."""
21+
if not members:
22+
raise ValueError("Cannot find common base of empty members list")
2123
filtered_mros = [
2224
[c for c in cls.__mro__ if is_model_class(c) and c is not BaseModel]
2325
for cls in members

packages/overture-schema-codegen/src/overture/schema/codegen/layout/type_collection.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,10 @@ def _visit(node: TypeInfo) -> None:
109109
if enum_id not in all_specs:
110110
all_specs[enum_id] = extract_enum(node.source_type)
111111
elif is_pydantic_type(node):
112-
assert node.source_type is not None # guaranteed by is_pydantic_type
112+
if node.source_type is None:
113+
raise TypeError(
114+
"is_pydantic_type returned True but source_type is None"
115+
)
113116
pid = TypeIdentity.of(node.source_type)
114117
if pid not in all_specs:
115118
all_specs[pid] = extract_pydantic_type(node.source_type)

packages/overture-schema-codegen/src/overture/schema/codegen/markdown/renderer.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,6 @@ def _format_example_value(value: object) -> str:
174174
if isinstance(value, bool):
175175
return "`true`" if value else "`false`"
176176

177-
if isinstance(value, datetime.datetime):
178-
return f"`{value.isoformat()}`"
179-
180177
if isinstance(value, datetime.date):
181178
return f"`{value.isoformat()}`"
182179

@@ -456,7 +453,9 @@ def _format_constraint(
456453
if cs.source_ref is None or cs.source_ref is newtype_ref:
457454
return _NewTypeConstraintRow(display=display)
458455

459-
assert cs.source_name is not None # source_ref and source_name are set together
456+
# source_ref and source_name are always set together
457+
if cs.source_name is None:
458+
return _NewTypeConstraintRow(display=display)
460459
source_identity = TypeIdentity(cs.source_ref, cs.source_name)
461460
source_link = ctx.resolve_link(source_identity) if ctx else None
462461
return _NewTypeConstraintRow(

packages/overture-schema-codegen/src/overture/schema/codegen/markdown/type_format.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,11 @@ def _plain_list_type(base: str, depth: int) -> str:
5454

5555
def _linked_type_identity(ti: TypeInfo) -> TypeIdentity | None:
5656
"""Return the TypeIdentity to use for a markdown link, or None for non-linked types."""
57-
if is_semantic_newtype(ti) and ti.newtype_ref is not None:
58-
assert ti.newtype_name is not None # guaranteed by is_semantic_newtype
57+
if (
58+
is_semantic_newtype(ti)
59+
and ti.newtype_ref is not None
60+
and ti.newtype_name is not None
61+
):
5962
return TypeIdentity(ti.newtype_ref, ti.newtype_name)
6063
if ti.kind in (TypeKind.ENUM, TypeKind.MODEL) and ti.source_type is not None:
6164
return TypeIdentity(ti.source_type, ti.base_type)
@@ -148,7 +151,6 @@ def format_type(
148151
# qualifier instead (e.g., Sources wrapping list[SourceItem] renders as
149152
# Sources (list)), since the list-ness is an implementation detail of the type.
150153
if ti.newtype_outer_list_depth > 0:
151-
assert ti.is_list # outer list layers are a subset of total list layers
152154
display = _wrap_list_n(display, ti.newtype_outer_list_depth)
153155
elif ti.is_list and ti.newtype_name is not None: # list is inside the NewType
154156
qualifiers.append("list")

0 commit comments

Comments
 (0)