Add validation for ISO 8601 durations to MeasurementType.duration
#16
Conversation
| @model_validator(mode="after") | ||
| def must_have_single_duration_if_instantaneous_method(self): | ||
| if self.method == "instantaneous" and "/" in self.duration: | ||
| raise ValueError( | ||
| "A measurement type object with 'instantaneous' method " | ||
| "MUST have a single duration." | ||
| ) | ||
|
|
||
| return self |
There was a problem hiding this comment.
Hesitant about this additional validation logic. I can't see anything formal in the specification that says the duration must be a single duration (e.g. PT0M) when the mode is instantaneous. However, this is the convention that I've seen in all EDR collections (including the JSON test data in this repository and the examples within the full EDR specification).
| def check_iso8601_duration(value: str) -> str: | ||
| if "/" in value: | ||
| parts = value.split("/") | ||
|
|
||
| if len(parts) != 2: | ||
| raise ValueError("Duration must have two parts if it contains a '/'") | ||
|
|
||
| duration_adapter.validate_python(parts[0]) | ||
| duration_adapter.validate_python(parts[1]) | ||
| else: | ||
| duration_adapter.validate_python(value) | ||
|
|
||
| return value |
There was a problem hiding this comment.
Thanks for making these improvements.
I don't think we want to keep supporting the duration field with a / in there, see my comment on your other PR.
There was a problem hiding this comment.
On the timedelta validation that you use, I think this relies on pydantic datetime validation, correct?
if I look here, it looks like this only supports part of the ISO8601 duration format:
[±]P[DD]DT[HH]H[MM]M[SS]S
I think this change would no longer allow periods like P1M, which is not consistent with the spec.
What is your view on this?
|
Apologies for jumping the gun and adding some comments before the PR is ready. |
No worries at all! It's all welcome feedback, and the comments raise interesting questions. Will take a proper look tomorrow. |
WIP, will move out of draft when ready for review.