Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-120266: Fix undefined_utcfromtimestamp name indatetimetester#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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy 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?
Conversation
| @@ -6223,7 +6223,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] | |||
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.
Was this causing test failures somewhere? I'm guessing not. It looks like the undefined identifier was introduced last year, in38dc3f2, 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 callingdatetime.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. 😄
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.
Yeah does this mean thatnondst_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 likelambda 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 ofutcfromtimestamp.
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.
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.
SmartManojMay 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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 in8da5ca4
| fordt,shift,prev_abbr,abbrintz.nondst_folds(): |
@sobolevn Even here?
_utcfromtimestamp name indatetimetested_utcfromtimestamp name indatetimetester| @@ -6223,7 +6223,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] | |||
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.
| yielddatetime.fromtimestamp(t,datetime.UTC),-shift,prev_ti[2],ti[2] | |
| yielddatetime.utcfromtimestamp(t),-shift,prev_ti[2],ti[2] |
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.
utcfromtimestamp is deprecated.
Uh oh!
There was an error while loading.Please reload this page.
also cc@ericsnowcurrently who recently reviewed similar PRs in this module :)
_utcfromtimestampis undefined indatetimetester#120266