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

Replace period with duration in MeasurementType #15

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

Conversation

mo-lukecarr
Copy link

@mo-lukecarr mo-lukecarr commented Dec 16, 2024

First off, great work on this library! It's been really useful for spec'ing out some EDR work and getting a lot of the EDR specification validated with minimal effort!


Historically, there has been confusion between the names period and duration for the field that denotes the ISO 8601 duration of a measurement type. This is noted in the TODO: comment on main within the MeasurementType model.

Now that the PR (opengeospatial/ogcapi-environmental-data-retrieval#577) has been merged on OGC's end, there is no ambiguity between period and duration moving forward. As seen in the linked PR, the examples have been changed to confirm that duration is the field name moving forward with EDR v1.2.

This PR changes the field name in the MeasurementType model from period to duration, and changes all references from period to duration in JSON test data to ensure that tests still pass.

I appreciate that this change aligns with an upcoming EDR specification version that isn't formally released, so I welcome your steer on when it would be best to merge.

Also, I recognise that this is technically a breaking change for the library, so any thoughts on how that can be mitigated are welcome. Do we still include the period field as a str | None that is deprecated (and completely removed at a later time)? Or do we just accept that it's a breaking change and switch from period to duration without a transitional phase?


Related to this PR, I'm happy to pick up the other TODO comment on this field which aims to add validation for ISO 8601 duration strings (rather than accepting any strings as the model currently does).

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.02%. Comparing base (ca0ab14) to head (5ecbcb4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #15   +/-   ##
=======================================
  Coverage   98.02%   98.02%           
=======================================
  Files          10       10           
  Lines         203      203           
=======================================
  Hits          199      199           
  Misses          4        4           
Flag Coverage Δ
unittests 98.02% <100.00%> (ø)

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.

@PaulVanSchayck
Copy link
Collaborator

Dear Luke, thank you for your very timely contribution. We were literally talking yesterday about making this change.

I think it will be fine to release this with 0.6.0 as breaking change. We only recently (#11) added measurementType. We think the production use of the field is still pretty minimal. But I will double check with @lukas-phaf for his opinion.

@lukas-phaf
Copy link
Collaborator

lukas-phaf commented Dec 18, 2024

Looks good to me. Let me check with @PaulVanSchayck when we want to merge and relrease.

@PaulVanSchayck
Copy link
Collaborator

Hey @mo-lukecarr

The changes look good to us. Thanks again. We were planning to do one more check on one of our EDR APIs, but failed to complete that before our Christmas holidays.

We'll continue to work on this the 6th of January. We maybe would like to include PR #17 into one breaking 0.6.0 release.

Happy holidays!

@mo-lukecarr
Copy link
Author

Hey @mo-lukecarr

The changes look good to us. Thanks again. We were planning to do one more check on one of our EDR APIs, but failed to complete that before our Christmas holidays.

We'll continue to work on this the 6th of January. We maybe would like to include PR #17 into one breaking 0.6.0 release.

Happy holidays!

I'm working over the holidays (except for the 25th and New Year), so I can crack on with #17 so it's ready for review in early January.

Happy Holidays, too!

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.

3 participants