-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Enhance and move ISO-8601 parser to coding.times #9899
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
Conversation
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
for more information, see https://pre-commit.ci
@dcherian Thanks for the review, I've addressed according your comments and suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kmuehlbauer—this looks nice. I guess so long as we accept a flexible number of digits in the year it will be hard to extend beyond five-digit years, which is totally fine for now.
Per @dcherian's TODO, I tried my hand at adding a hypothesis strategy for generating cftime objects in kmuehlbauer#2.
Co-authored-by: Spencer Clark <spencerkclark@gmail.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take the opportunity and add some typing.
But looks good, thanks.
Co-authored-by: Michael Niklas <mick.niklas@gmail.com>
for more information, see https://pre-commit.ci
Thanks @headtr1ck for the review and typing addendum. |
Thanks @spencerkclark for reviewing and the additions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kmuehlbauer! Though it passes CI, I'm happy to iterate further on the hypothesis portion in this or a follow-up PR if needed. It's fairly low-stakes / orthogonal to the main contribution.
return self.min_value + datetime.timedelta(microseconds=result) | ||
|
||
|
||
class CFTimeStrategyISO8601(st.SearchStrategy): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spencerkclark very nice!
I think the idiomatic way is to use composite but we can do that later.
* main: (63 commits) Fix zarr upstream tests (pydata#9927) Update pre-commit hooks (pydata#9925) split out CFDatetimeCoder, deprecate use_cftime as kwarg (pydata#9901) dev whats-new (pydata#9923) Whats-new 2025.01.0 (pydata#9919) Silence upstream Zarr warnings (pydata#9920) time coding refactor (pydata#9906) fix warning from scipy backend guess_can_open on directory (pydata#9911) Enhance and move ISO-8601 parser to coding.times (pydata#9899) Edit serialization error message (pydata#9916) friendlier error messages for missing chunk managers (pydata#9676) Bump codecov/codecov-action from 5.1.1 to 5.1.2 in the actions group (pydata#9915) Rewrite interp to use `apply_ufunc` (pydata#9881) Skip dask rolling (pydata#9909) Explicitly configure ReadTheDocs build to use conf.py (pydata#9908) Cache pre-existing Zarr arrays in Zarr backend (pydata#9861) Optimize idxmin, idxmax with dask (pydata#9800) remove unused "type: ignore" comments in test_plot.py (fixed in matplotlib 3.10.0) (pydata#9904) move scalar-handling logic into `possibly_convert_objects` (pydata#9900) Add missing DataTree attributes to docs (pydata#9876) ...
This enhances the ISO-8601 parser to check for signed years (eg, -0300 or +0010) as well as 5 digit years (eg. -10000, +20000, 10000). The relevant parsing code is moved to coding.times (to prevent circular import) as these functionality will be used in #9618 within coding.times.
whats-new.rst