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

gh-102450: Add ISO-8601 alternative for midnight to fromisoformat() calls. #105856

Merged
merged 19 commits into from
Sep 25, 2024

Conversation

TizzySaurus
Copy link
Contributor

@TizzySaurus TizzySaurus commented Jun 16, 2023

Closes #102450.
Closes #124257.

I've updated _datetimemodule.c and _pydatetime.py such that calls to Python's datetime.time.fromisoformat and datetime.datetime.fromisoformat both allow providing the 24:00 ISO-8601 alternative to midnight, but the class constructors themselves do not (as per this comment from @pganssle).

Screenshots of the C code working:
datetime.time.fromisoformat()
datetime.datetime.fromisoformat()

I've also done the same tests for the _pydatetime module and everything appears to be working.

Also updated the datetime tests accordingly, to ensure that datetime.time.fromisoformat("24:00:00.000000") is parsed as datetime.time(0, 0, 0, 0) etc. and the cases that should error (such as datetime.time.fromisoformat("24:30")) do indeed error.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jun 16, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@wookie184
Copy link
Contributor

It seems I don't need to edit the Python code, but just the C?

The PEP that explains it was linked on the issue: #102450 (comment)

Basically, (I believe) you need to implement it in both places but only the C version is used by CPython. If you add tests they should run on both implementations so you can ensure you've done things correctly. You should also be able to import _pydatetime directly to test it manually.

@TizzySaurus TizzySaurus reopened this Jun 17, 2023
@TizzySaurus
Copy link
Contributor Author

TizzySaurus commented Jun 17, 2023

Thanks @wookie184, that makes sense 😄

I do have a query though: the tests that I added in 311f914 are actually passing (./python.exe -m test -v datetimetester.py), despite me not having implemented the feature into the python code yet. My understanding of PEP399 was that the one file tested both the Python and C implementation, but it seems that datetimetester.py actually only tests the C code? Do I have to write the tests for the Python implementation of the library somewhere else?

EDIT: I've since added the Python implementation, but am still not convinced that the Python implementation is getting tested locally (seems to be here on GitHub though).

@TizzySaurus
Copy link
Contributor Author

TizzySaurus commented Jun 17, 2023

I've also just noticed that the datetime and _pydatetime modules give different errors when specifying an invalid day:

>>> from datetime import datetime as c_datetime
>>> c_datetime.fromisoformat("2023-01-32T24:00:00")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: day is out of range for month

vs

>>> from _pydatetime import datetime as py_datetime
>>> py_datetime.fromisoformat("2023-01-32T24:00:00")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/xxx/Documents/personal/cpython/Lib/_pydatetime.py", line 1906, in fromisoformat
    return cls(*(date_components + time_components))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/xxx/Documents/personal/cpython/Lib/_pydatetime.py", line 1717, in __new__
    year, month, day = _check_date_fields(year, month, day)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/xxx/Documents/personal/cpython/Lib/_pydatetime.py", line 545, in _check_date_fields
    raise ValueError('day must be in 1..%d' % dim, day)
ValueError: ('day must be in 1..31', 32)

Note how _pydatetime's error message specifies the actual day range for that month. Is this something I should add to the C implementation whilst I'm here? Should just be case of swapping PyErr_SetString() with PyErr_Format() -- although not sure how to replicate it being a tuple.

@pganssle
Copy link
Member

Note how _pydatetime's error message specifies the actual day range for that month. Is this something I should add to the C implementation whilst I'm here? Should just be case of swapping PyErr_SetString() with PyErr_Format() -- although not sure how to replicate it being a tuple.

We can handle this separately.

Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

This looks great! Do you want to add yourself to the ACKS file and/or to the patch name, so you get credit? (This is optional)

Also can you rebase against main?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I like how invalid inputs (month 13, non-zero minute, etc.) are checked: it's smart (and keep the code short) :-) I also like tests which seem to cover all cases.

About the feature itself, I trust @pganssle's opinion to decide if we should implement it or not.

@TizzySaurus
Copy link
Contributor Author

TizzySaurus commented Sep 25, 2024

@pganssle Also can you rebase against main?

Think I've done this correctly, so hopefully should be good now 🙂

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Oh nice, the code handles also well the maximum date:

$ ./python
>>> import datetime
>>> datetime.datetime.fromisoformat('9999-12-31T24:00:00')
ValueError: year 10000 is out of range

Lib/test/datetimetester.py Show resolved Hide resolved
@pganssle pganssle merged commit b0c6cf5 into python:main Sep 25, 2024
37 checks passed
emilyemorehouse added a commit to lysnikolaou/cpython that referenced this pull request Sep 26, 2024
* main: (69 commits)
  Add "annotate" SET_FUNCTION_ATTRIBUTE bit to dis. (python#124566)
  pythongh-124412: Add helpers for converting annotations to source format (python#124551)
  pythongh-119180: Disallow instantiation of ConstEvaluator objects (python#124561)
  For-else deserves its own section in the tutorial (python#123946)
  Add 3.13 as a version option to the crash issue template (python#124560)
  pythongh-123242: Note that type.__annotations__ may not exist (python#124557)
  pythongh-119180: Make FORWARDREF format look at __annotations__ first (python#124479)
  pythonGH-58058: Add quick reference for `ArgumentParser` to argparse docs (pythongh-124227)
  pythongh-41431: Add `datetime.time.strptime()` and `datetime.date.strptime()` (python#120752)
  pythongh-102450: Add ISO-8601 alternative for midnight to `fromisoformat()` calls. (python#105856)
  pythongh-124370: Add "howto" for free-threaded Python (python#124371)
  pythongh-121277: Allow `.. versionadded:: next` in docs (pythonGH-121278)
  pythongh-119400:  make_ssl_certs: update reference test data automatically, pass in expiration dates as parameters python#119400  (pythonGH-119401)
  pythongh-119180: Avoid going through AST and eval() when possible in annotationlib (python#124337)
  pythongh-124448: Update Windows builds to use Tcl/Tk 8.6.15 (pythonGH-124449)
  pythongh-123884 Tee of tee was not producing n independent iterators (pythongh-124490)
  pythongh-124378: Update test_ttk for Tcl/Tk 8.6.15 (pythonGH-124542)
  pythongh-124513: Check args in framelocalsproxy_new() (python#124515)
  pythongh-101100: Add a table of class attributes to the "Custom classes" section of the data model docs (python#124480)
  Doc: Use ``major.minor`` for documentation distribution archive filenames (python#124489)
  ...
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.

Invalid datetime.time.fromisoformat datetime library doesn't support valid ISO-8601 alternative for midnight
5 participants