-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-130718: Normalize edge cases in datetime.timestamp
and datetime.astimezone
#130752
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5415,6 +5415,7 @@ | |
{ | ||
struct tm local_time; | ||
time_t t; | ||
long long old_u = u; | ||
u -= epoch; | ||
t = u; | ||
if (t != u) { | ||
|
@@ -5424,7 +5425,14 @@ | |
} | ||
if (_PyTime_localtime(t, &local_time) != 0) | ||
return -1; | ||
return utc_to_seconds(local_time.tm_year + 1900, | ||
|
||
// Check edge cases | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be more specific — what this code does is very opaque, so we should have a "why" comment here. Something like "When the year is outside the allowed year boundaries, return the original utc timestamp because ". |
||
int year = local_time.tm_year + 1900; | ||
if (year < MINYEAR || year > MAXYEAR) { | ||
Py_DECREF(year); | ||
Check warning on line 5432 in Modules/_datetimemodule.c
|
||
donBarbos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return old_u; | ||
} | ||
return utc_to_seconds(year, | ||
local_time.tm_mon + 1, | ||
local_time.tm_mday, | ||
local_time.tm_hour, | ||
|
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.
I don't love that the original style violates the one-assert-per-test style, and we should probably not continue that.
Let's move this into its own method, like
test_astimezone_max
andtest_astimezone_min
. We can test the values neardatetime.min
anddatetime.max
separately, and useself.subTest
to try a few different values for time zone.I'm not sure if
@support.run_with_tz
can be parameterized, but if it can we should try it on a few different time zones.