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

mo-lukecarr · 2024-12-16T17:50:58Z

Addresses and resolves the existing TODO comment for validating temporal list lengths.

Currently, the interval field of the Temporal data class doesn't validate the number of datetimes that are provided. Per the specification, each interval should consist of exactly two datetime strings, so we should change the interval from:

interval: List[List[AwareDatetime]]

to:

interval: List[Annotated[List[AwareDatetime], Len(min_length=2, max_length=2)]]

src/edr_pydantic/extent.py

mo-lukecarr · 2024-12-16T17:54:25Z

I appear to get an error in IntelliJ on the use of Len(...) which says Generics should be specified through square brackets. However, tests are still passing locally and a quick search suggests that this is a known issue with JetBrains' inspections (e.g. non-issue).

PaulVanSchayck · 2024-12-17T14:05:19Z

Thanks for this draft PR. You probably need to add this snippet to make Python 3.8 happy:

if sys.version_info < (3, 9):
    from typing_extensions import Annotated
else:
    from typing import Annotated

mo-lukecarr · 2024-12-18T08:48:07Z

Thanks for this draft PR. You probably need to add this snippet to make Python 3.8 happy:
if sys.version_info < (3, 9):
    from typing_extensions import Annotated
else:
    from typing import Annotated

My impression (maybe wrong!) is that typing_extensions can be a drop-in for all versions, so I could amend the import to just use from typing_extensions import Annotated for all versions, rather than checking sys.version_info and conditionally importing.

Or is there a specific preference to use this approach? Happy to do so if there is!

PaulVanSchayck · 2025-01-06T15:29:58Z

Thanks for this draft PR. You probably need to add this snippet to make Python 3.8 happy:
if sys.version_info < (3, 9):
    from typing_extensions import Annotated
else:
    from typing import Annotated
My impression (maybe wrong!) is that typing_extensions can be a drop-in for all versions, so I could amend the import to just use from typing_extensions import Annotated for all versions, rather than checking sys.version_info and conditionally importing.

Or is there a specific preference to use this approach? Happy to do so if there is!

Ah! Didn't know this :) I guess I don't mind either way too much. Maybe the version check is a bit more explicit, but yours is fine as well.

Could you still add the test for a failing validation case?

mo-lukecarr · 2025-01-07T10:08:25Z

Thanks for this draft PR. You probably need to add this snippet to make Python 3.8 happy:
if sys.version_info < (3, 9):
    from typing_extensions import Annotated
else:
    from typing import Annotated
My impression (maybe wrong!) is that typing_extensions can be a drop-in for all versions, so I could amend the import to just use from typing_extensions import Annotated for all versions, rather than checking sys.version_info and conditionally importing.
Or is there a specific preference to use this approach? Happy to do so if there is!
Ah! Didn't know this :) I guess I don't mind either way too much. Maybe the version check is a bit more explicit, but yours is fine as well.

Could you still add the test for a failing validation case?

Got some time set aside this afternoon to wrap up this PR. I don't want to block the 0.6.0 release much longer!

codecov · 2025-01-16T12:09:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.05%. Comparing base (ca0ab14) to head (60bd808).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   98.02%   98.05%   +0.02%     
==========================================
  Files          10       10              
  Lines         203      206       +3     
==========================================
+ Hits          199      202       +3     
  Misses          4        4

Flag	Coverage Δ
unittests	`98.05% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mo-lukecarr · 2025-01-16T12:12:19Z

@PaulVanSchayck, this PR is ready for review when you have a spare minute.

I've added one bad test case for the validation logic, with a temporal interval that contains a single datetime (rather than a pair). Is this satisfactory for what we'd like to cover in our tests?

The successful tests cases already exist in the repo and cover instances where a pair of datetimes are provided as the interval.

I've actually added another commit that refactors the interval length validation so we can actually unit test how the Pydantic validation logic is configured. Is this overkill, or a suitable approach for testing the logic works as intended?

mo-lukecarr · 2025-01-16T12:23:13Z

tests/test_extent.py

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?

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

lukas-phaf

Thanks for completing this!

A couple of small comments.

lukas-phaf · 2025-01-20T13:06:24Z

src/edr_pydantic/extent.py

 from pydantic import AwareDatetime
+from typing_extensions import Annotated


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?

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

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.

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.

lukas-phaf · 2025-01-20T13:14:53Z

src/edr_pydantic/extent.py

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


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

Ah, I see, for testing purposes.

lukas-phaf · 2025-01-20T13:21:40Z

tests/test_extent.py

+    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"


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?

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

feat: validation for number of datetimes in Temporal.interval

8e74823

mo-lukecarr commented Dec 16, 2024

View reviewed changes

src/edr_pydantic/extent.py Outdated Show resolved Hide resolved

fix: py3.8 compatibility with import from typing_extensions

89a0725

PaulVanSchayck mentioned this pull request Dec 20, 2024

Replace period with duration in MeasurementType #15

Merged

test: added test case for invalid temporal interval

df32b50

mo-lukecarr marked this pull request as ready for review January 16, 2025 11:57

mo-lukecarr added 2 commits January 16, 2025 12:06

fix: reordered imports to satisfy pre-commit

0c258fa

chore: added newline to end of test data file

df71de4

test: refactored temporal interval length for more precise unit testing

60bd808

mo-lukecarr commented Jan 16, 2025

View reviewed changes

mo-lukecarr mentioned this pull request Jan 16, 2025

Add tests for 100% coverage #20

Draft

lukas-phaf reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mo-lukecarr commented Dec 16, 2024 •

edited

Loading

mo-lukecarr commented Dec 16, 2024

PaulVanSchayck commented Dec 17, 2024

mo-lukecarr commented Dec 18, 2024

PaulVanSchayck commented Jan 6, 2025

mo-lukecarr commented Jan 7, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

mo-lukecarr commented Jan 16, 2025 •

edited

Loading

mo-lukecarr Jan 16, 2025 •

edited

Loading

lukas-phaf Jan 20, 2025

lukas-phaf left a comment

lukas-phaf Jan 20, 2025

lukas-phaf Jan 21, 2025

mo-lukecarr Jan 21, 2025 •

edited

Loading

lukas-phaf Jan 21, 2025

lukas-phaf Jan 20, 2025

lukas-phaf Jan 20, 2025

lukas-phaf Jan 20, 2025

lukas-phaf Jan 20, 2025

		from pydantic import AwareDatetime
		from typing_extensions import Annotated

		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"

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

Are you sure you want to change the base?

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

Conversation

mo-lukecarr commented Dec 16, 2024 • edited Loading

mo-lukecarr commented Dec 16, 2024

PaulVanSchayck commented Dec 17, 2024

mo-lukecarr commented Dec 18, 2024

PaulVanSchayck commented Jan 6, 2025

mo-lukecarr commented Jan 7, 2025

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

mo-lukecarr commented Jan 16, 2025 • edited Loading

mo-lukecarr Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-phaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mo-lukecarr Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

mo-lukecarr commented Dec 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading

mo-lukecarr commented Jan 16, 2025 •

edited

Loading

mo-lukecarr Jan 16, 2025 •

edited

Loading

mo-lukecarr Jan 21, 2025 •

edited

Loading