-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add a pydantic model which encapsulates required BMI time meta data #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good! I think we can improve on the one pydantic
model by using typing.Literal
s instead of a custom validator. LMK if I wasn't clear about that or you would like to provide an example.
bmi_sdk/bmi_sdk/bmi_time.py
Outdated
# TODO extracted from looking throug the xml files | ||
# https://docs.unidata.ucar.edu/udunits/current/#Database | ||
# need to verify this is a complete set... | ||
valid_units = ["s", "sec", "second", "seconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would instead make this into a typing.Literal
that can be used here or else where. Then there is also no need for this validator, since pydantic will handle it for you.
@hellkite500, just as a heads up, I changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Nice addition with the Literals
This model can be used in BMI implementations to store related time components and validate the BMI constraints of these meta data.