gh-53502: add a new option aware_datetime in plistlib to loads or dumps aware datetime. #113363
Conversation
aisk
changed the title
There was a problem hiding this comment.
Please update the documentation, add versionchanged directives and a What's New entry.
Add tests for serializing a datatime object with UTC timezone (it should never be skipped), a naive datatime object with aware_datetime=True and an aware datatime object with aware_datetime=False (if it was not already covered).
|
My first thought was: is the new parameter needed when writing plist files. And indeed, it is: we currently completely ignore the tzinfo. That's something we can warn users about, but that's for a different PR. |
Tests and documents have been added, except for writing a aware datetime with Line 781 in 0c57454 TypeError.
Therefore, the current code does not support write a aware datetime with |
Can we raise a |
TypeError is also result. We should test that this is rejected in binary format rather than producing unexpected result. Emitting a DeprecatedWarning is a good ide, but I think that it should be in other issue, and to be nice to users, it is better to wait few versions before adding runtime warning, so silencing it by passing |
|
@serhiy-storchaka thanks for the review, I updated all the codes with your comments. |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Co-authored-by: Ronald Oussoren <[email protected]>
Lib/test/test_plistlib.py
Outdated
| def test_dump_aware_datetime(self): | ||
| dt = datetime.datetime(2345, 6, 7, 8, 9, 10, | ||
| tzinfo=zoneinfo.ZoneInfo("America/Los_Angeles")) | ||
| for fmt in ALL_FORMATS: | ||
| s = plistlib.dumps(dt, fmt=fmt, aware_datetime=True) | ||
| self.assertEqual(plistlib.loads(s, fmt=fmt), | ||
| datetime.datetime(2345, 6, 7, 15, 9, 10)) |
There was a problem hiding this comment.
I do not like the results of this test. It silently ignores the time zone and returns a datetime not equal to the origin. It was expected that aware_datetime=True is a stricter option.
There was a problem hiding this comment.
Updated to loads the result with aware_datetime=True, and compared the loads result with the original dt.
test_dump_utc_aware_datetime have the same issue and updated too.
There was a problem hiding this comment.
Great. Now I see that it works as expected.
But it would be nice to test also that tzinfo is UTC in the loaded datetime.
There was a problem hiding this comment.
Thanks for the review, updated.
1. Use 32-bit compatible date in test_dump_naive_datetime_with_aware_datetime_option 2. test_dump_naive_datetime_with_aware_datetime_option: Writing non-aware datetimes with aware_datetime==True leads to the time being off by twice the timezone offset from UTC
|
#113627 fixes the issue with test_dump_naive_datetime_with_aware_datetime_option for me, test_dump_naive_datetime_with_aware_datetime_option on AIX should be fixed by using a date that's in range for 32-bit time_t. |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
There are two problems:
I'm not entirely happy about the second change, but don't quite understand what's causing the problem there. I'm leaving the issue open to further investigate what's going on here. At first I thought that the offset was double the expected offset, but's that not correct either. |
|
DST. |
|
|
See #113645. |
…or dumps aware datetime. (python#113363) * add options to loads and dumps aware datetime in plistlib
* pythongh-53502: Fixes for tests in pythongh-113363 * Use 32-bit compatible date in test_dump_naive_datetime_with_aware_datetime_option * Saving non-aware datetimes will use the old behaviour regardless of the aware_datimetime setting
…or dumps aware datetime. (python#113363) * add options to loads and dumps aware datetime in plistlib
* pythongh-53502: Fixes for tests in pythongh-113363 * Use 32-bit compatible date in test_dump_naive_datetime_with_aware_datetime_option * Saving non-aware datetimes will use the old behaviour regardless of the aware_datimetime setting
…or dumps aware datetime. (python#113363) * add options to loads and dumps aware datetime in plistlib
* pythongh-53502: Fixes for tests in pythongh-113363 * Use 32-bit compatible date in test_dump_naive_datetime_with_aware_datetime_option * Saving non-aware datetimes will use the old behaviour regardless of the aware_datimetime setting
Added a new option
aware_datetimeinload/dumpand related functions, and the default value isFalse. In this case, there is no behavior changes from old versions of this library for compatible issue.If set
aware_datetimetoTrue, theloadand related functions will createdatetimewithtzinfo=UTC. Thedumpand related functions will convert the datetime to UTC bebore dump.