-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
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.
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?
Conversation
| 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, 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. 😄
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 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.
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.
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 in 8da5ca4
| for dt, shift, prev_abbr, abbr in tz.nondst_folds(): |
@sobolevn Even here?
_utcfromtimestamp name in datetimetested_utcfromtimestamp name in datetimetester
| 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.
| yield datetime.fromtimestamp(t, datetime.UTC), -shift, prev_ti[2], ti[2] | |
| yield datetime.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.
also cc @ericsnowcurrently who recently reviewed similar PRs in this module :)
_utcfromtimestampis undefined indatetimetester#120266