Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert from BaseModel to static jsonschema + TypedDict #341

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

evalott100
Copy link
Contributor

Closes #337

@evalott100 evalott100 self-assigned this Dec 17, 2024
@evalott100 evalott100 force-pushed the 337-add-basemodel-support branch from 73a8dff to 56b88ef Compare December 18, 2024 09:38
@evalott100 evalott100 force-pushed the 337-add-basemodel-support branch from 56b88ef to 5f1d747 Compare December 18, 2024 11:17
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! This is looking pretty good

src/event_model/__init__.py Outdated Show resolved Hide resolved
src/event_model/schemas/bulk_events.json Outdated Show resolved Hide resolved
src/event_model/generate/type_wrapper.py Outdated Show resolved Hide resolved
@evalott100 evalott100 force-pushed the 337-add-basemodel-support branch from 3c618ec to fda7e64 Compare December 18, 2024 12:16
@evalott100 evalott100 force-pushed the 337-add-basemodel-support branch 2 times, most recently from 59a86e9 to fdf2837 Compare December 19, 2024 10:56
@evalott100 evalott100 marked this pull request as ready for review December 19, 2024 10:56
@evalott100 evalott100 force-pushed the 337-add-basemodel-support branch from fdf2837 to 3e827a2 Compare December 19, 2024 11:02
@evalott100 evalott100 force-pushed the 337-add-basemodel-support branch from 315bf60 to 35645e1 Compare December 19, 2024 14:24
@evalott100 evalott100 requested a review from coretl December 19, 2024 15:13
Comment on lines -20 to -44
DocumentType = Union[
Type[Datum],
Type[DatumPage],
Type[Event],
Type[EventDescriptor],
Type[EventPage],
Type[Resource],
Type[RunStart],
Type[RunStop],
Type[StreamDatum],
Type[StreamResource],
]

ALL_DOCUMENTS: Tuple[DocumentType, ...] = (
Datum,
DatumPage,
Event,
EventDescriptor,
EventPage,
Resource,
RunStart,
RunStop,
StreamDatum,
StreamResource,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file probably has to go back to how it was, at least for the presence of DocumentType and ALL_DOCUMENTS

Comment on lines +13 to +21
"anyOf": [
{
"type": "object"
},
{
"type": "null"
}
],
"default": null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is actually saying that downstream clients now need to cope with the presence of this key, and its value being nul...

Comment on lines -135 to -143
data_keys: NotRequired[
Annotated[
Dict[str, DataKey],
Field(
description="This describes the data stored alongside it in this "
"configuration object."
),
]
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...it used to be NotRequired[Dict]...

class Configuration(BaseModel):
model_config = ConfigDict(extra="forbid")
data: Annotated[
Union[Dict[str, Any], None],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and now it's Union[Dict, None].

Is there any way to persuade pydantic to treat None as NotRequired in the JSON schema?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BaseModel documents and change to schema generation
2 participants