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.

Sign up for GitHub

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

Add validation for number of datetimes in Temporal.interval #17

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

Add validation for number of datetimes in Temporal.interval #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e74823
feat: validation for number of datetimes in `Temporal.interval`
mo-lukecarr Dec 16, 2024
89a0725
fix: py3.8 compatibility with import from `typing_extensions`
mo-lukecarr Dec 18, 2024
df32b50
test: added test case for invalid temporal interval
mo-lukecarr Jan 16, 2025
0c258fa
fix: reordered imports to satisfy pre-commit
mo-lukecarr Jan 16, 2025
df71de4
chore: added newline to end of test data file
mo-lukecarr Jan 16, 2025
60bd808
test: refactored temporal interval length for more precise unit testing
mo-lukecarr Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/edr_pydantic/extent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
from typing import Optional
from typing import Union

from annotated_types import Len
from pydantic import AwareDatetime
from typing_extensions import Annotated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is typing_extensions an externel dependency or not. It is not clear to me from the docs.

Should we make it explicit we are using this by adding it to dependencies in pyproject.toml, instead of relying on pydantic to install it for us?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should just do the same as pydantic does?
https://github.com/pydantic/pydantic/blob/main/pyproject.toml#L46

Copy link
Contributor Author

@mo-lukecarr mo-lukecarr Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'd suggest that because pydantic installs it for us, and this library is so closely coupled to pydantic, then maybe we don't need to be explicit about it? We'd also be able to just use the version provided by pydantic, rather than possibly bringing in two versions of typing_extensions.

Although, I'm happy to install explicitly as pydantic does, if that's the desired approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to explicitly mention packages that you import, as there is no guarantee that pydantic will keep using/providing it.

I think there can only be one version of a package in a specific Python environment, and that consistency in the requirements is checked during installation. So as long as we use a greater then package requirement, it should always work. Specifically, if we use typing-extensions>=4.12.2, we should be good.


from .base_model import EdrBaseModel

Expand All @@ -12,9 +14,11 @@ class Spatial(EdrBaseModel):
crs: str


IntervalLen = Len(min_length=2, max_length=2)


class Temporal(EdrBaseModel):
# TODO: Validate this list has two items (C.7. Temporal Object)
interval: List[List[AwareDatetime]]
interval: List[Annotated[List[AwareDatetime], IntervalLen]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why define IntervalLen instead of using the Len(...) in-place?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, for testing purposes.

# TODO: Validate this is a list of ISO 8601 single time, ISO 8601 time duration or ISO 8601 interval
values: List[str]
trs: str
Expand Down
11 changes: 11 additions & 0 deletions tests/test_data/temporal-interval.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"interval": [
[
"2020-04-19T11:00:00Z"
]
],
"values": [
"2020-04-19T11:00:00Z/2020-06-30T09:00:00Z"
],
"trs": "TIMECRS[\"DateTime\",TDATUM[\"Gregorian Calendar\"],CS[TemporalDateTime,1],AXIS[\"Time (T)\",future]]"
}
6 changes: 5 additions & 1 deletion tests/test_edr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from edr_pydantic.collections import Collections
from edr_pydantic.collections import Instance
from edr_pydantic.extent import Extent
from edr_pydantic.extent import Temporal
from edr_pydantic.parameter import Parameter
from edr_pydantic.unit import Unit
from pydantic import RootModel
Expand Down Expand Up @@ -35,7 +36,10 @@ def test_happy_cases(file_name, object_type):
assert object_type.model_validate_json(json_string).model_dump_json(exclude_none=True) == json_string


error_cases = [("label-or-symbol-unit.json", Unit, r"Either 'label' or 'symbol' should be set")]
error_cases = [
("label-or-symbol-unit.json", Unit, r"Either 'label' or 'symbol' should be set"),
("temporal-interval.json", Temporal, r"List should have at least 2 items after validation"),
]


@pytest.mark.parametrize("file_name, object_type, error_message", error_cases)
Expand Down
6 changes: 6 additions & 0 deletions tests/test_extent.py
View file
Open in desktop
Copy link
Contributor Author

@mo-lukecarr mo-lukecarr Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this testing logic should be in a separate file (rather than test_edr.py) because that file is focused on the different happy/error test cases represented as JSON files.

Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In principle I agree, but I don't think these specific tests are needed.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from edr_pydantic.extent import IntervalLen


def test_interval_len():
assert IntervalLen.min_length == 2, "Temporal intervals must have at least 2 items"
assert IntervalLen.max_length == 2, "Temporal intervals must have at most 2 items"
Comment on lines +5 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am probably missing something, but it feels like you are testing that Python can set a class member...

I much prefer the approach with temporal-interval.json. Why not add a test there with 3 elements to ensure that both lower and upper bound are unchanged?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see now you already mention this in your comments.
As you can see from my comment above, I think this is overkill...

Loading