ESMValGroup / ESMValCore

ESMValCore: A community tool for pre-processing data from Earth system models in CMIP and running analysis scripts.
https://www.esmvaltool.org
Apache License 2.0
42 stars 38 forks source link

[v2.11.1] reformat datetime strings be in line with new `isodate==0.7.0` and actual ISO8601 and pin `isodate>=0.7.0` #2546

Closed valeriupredoi closed 1 month ago

valeriupredoi commented 1 month ago

Description

Closes #2545

As of the new isodate=0.7.0 released 11 hours ago, the package is a bit stricter what is actually iingesting as ISO8601 - in other words, @bouweandela was correct (like always :grin: )

OK so @schlunma raised a few good points about how we handle these isodate strings, in his comments below; as a response, here are some key elements belonging to ISO8601 from https://www.digi.com/resources/documentation/digidocs/90001488-13/reference/r_iso_8601_duration_format.htm


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the ๐Ÿ›  Technical or ๐Ÿงช Scientific review.


To help with the number pull requests:

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.91%. Comparing base (da3440e) to head (2b80610). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2546 +/- ## ======================================= Coverage 94.91% 94.91% ======================================= Files 251 251 Lines 14261 14264 +3 ======================================= + Hits 13536 13539 +3 Misses 725 725 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

valeriupredoi commented 1 month ago

I was going :exploding_head: completely missing the key fact that there is parse_date() and parse_datetime() that are different - one errors when the other doesn't, and vice-versa - I need to add to the _check.py's test the bit I added, but that tomorrow :beer:

valeriupredoi commented 1 month ago

@schlunma cheers, Manu! Yes, it is backwards compatible bc the Circle tests still use the older isodate=0.6.1; if we want to, we can just pin isodate>=0.7.0 in our env - the previous version is ancient anyhow; more pressing is you comment about the other bits where isodate.parse_ ... is invoked (well spotted!) - I reckon we ought to check, but am not fully sold on it

valeriupredoi commented 1 month ago

OK I've now pretty much refactored all the handling of isodate strings (apart from the ones in local.py that are correctly handled) , added a couple tests, and pinned to isodate>=0.7.0 - the old package, apart from being very old, has some funny ways of handling parsing exceptions, and those better in the new 0.7.0, hence some of our negative tests are not compatible anymore with the old one (in essence the same fail, but contorted exceptions for 0.6.1). May goal was no not catch any iso errors and filter them through our stack, but rather, let them immediately be raised from isodate - lot more straightforward that way :beer:

valeriupredoi commented 1 month ago

the upstream dev deps test fails bc of esgf-pyclient and that means I have to go there and fix whatever broke there too - not in a rush though, we can get this in no problem in the meantime, just another bit for me to look into :confounded:

bouweandela commented 1 month ago

It may have been a network issue, re-running tests seems to have solved it.

valeriupredoi commented 1 month ago

cheers @bouweandela :beers: You here to review as well, or just just passing by :grin:

bouweandela commented 1 month ago

Just passing by, it looks like @schlunma is doing an excellent job reviewing.

valeriupredoi commented 1 month ago

Danke viel Manu :beer: