Skip to content

gh-120266: Fix undefined _utcfromtimestamp name in datetimetester #120267

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

gh-120266: Fix undefined _utcfromtimestamp name in datetimetester #120267

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
397a5f3
gh-120266: Fix undefined `_utcfromtimestamp` name in `datetimetested`
sobolevn Jun 8, 2024
8da5ca4
Merge branch 'main' into issue-120266
sobolevn May 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/test/datetimetester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6604,7 +6604,7 @@ def nondst_folds(self):
for (_, prev_ti), (t, ti) in pairs(zip(self.ut, self.ti)):
shift = ti[0] - prev_ti[0]
if shift < ZERO and ti[1] == prev_ti[1]:
yield _utcfromtimestamp(datetime, t,), -shift, prev_ti[2], ti[2]
yield datetime.fromtimestamp(t, datetime.UTC), -shift, prev_ti[2], ti[2]
Copy link
Member

Choose a reason for hiding this comment

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

Was this causing test failures somewhere? I'm guessing not. It looks like the undefined identifier was introduced last year, in 38dc3f2, so I would have expected failures then. Perhaps we could drop the method entirely. I'm sure @pganssle would have a better idea on what's going on here.

As to the specific fix here, that seems valid. However, sometimes in tests we want to avoid specific code paths for certain test cases. It isn't clear to me if there would be any reason for to avoid calling datetime.fromtimestamp() directly here. If not then this change should be good to go.

Regardless, I definitely agree that it looks like we can't just leave _utcfromtimestamp there. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Yeah does this mean that nondst_folds is not finding anything? Or is it possible that we're just not using this whole class for something?

In any case, this is not the right thing to do, because it is returning an aware datetime. I think _utcfromtimestamp was supposed to be an alias for something like lambda t: datetime(1970, 1, 1) + timedelta(seconds=t). I am fuzzy on the details now, but I think this kind of thing was one of the rare legitimate uses of utcfromtimestamp.

Copy link
Member Author

Choose a reason for hiding this comment

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

nondst_folds is not used at all. Quoting myself:

This problem is not detected by tests, because this looks like a in-test helper for module maintainers. It is not used as of right now.

Copy link

@SmartManoj SmartManoj May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In any case, this is not the right thing to do, because it is returning an aware datetime.

Side effects?

nondst_folds is not used at all.

cpython/Lib/test/datetimetester.py

Line 6614 in 8da5ca4

for dt, shift, prev_abbr, abbr in tz.nondst_folds():

@sobolevn Even here?

Copy link

Choose a reason for hiding this comment

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

Suggested change
yield datetime.fromtimestamp(t, datetime.UTC), -shift, prev_ti[2], ti[2]
yield datetime.utcfromtimestamp(t), -shift, prev_ti[2], ti[2]

Discussion

Copy link
Member Author

Choose a reason for hiding this comment

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

utcfromtimestamp is deprecated.


@classmethod
def print_all_nondst_folds(cls, same_abbr=False, start_year=1):
Expand Down
Loading