Replace to-be-deprecated utcnow() datetime calls with aware now() calls - #475
Replace to-be-deprecated utcnow() datetime calls with aware now() calls#475jenstroeger wants to merge 1 commit into
Conversation
jenstroeger
left a comment
jenstroeger
left a comment
There was a problem hiding this comment.
A few comments below to continue the conversation from issue #473.
| v = datetime.utcnow() + v | ||
| v = datetime.now(tz=zoneinfo.ZoneInfo("UTC")) + v |
There was a problem hiding this comment.
The string formatting below doesn’t care that the datetime object is aware here.
| if isinstance(v, datetime): | ||
| v = v.astimezone(zoneinfo.ZoneInfo("UTC")).timetuple() | ||
| elif isinstance(v, date): | ||
| v = v.timetuple() |
There was a problem hiding this comment.
The function can receive bytes, str, int but it doesn’t have an else branch to catch any other unexpected type. If for some reason a datetime is passed in then we’ll want to make sure it’s in UTC, hence the adjustment. Also, unless a date object is passed in there shouldn’t be a need to handle those but cookies should always contain a date & time AFAIK.
There was a problem hiding this comment.
This change would require a test. For the specific utcnow issue I think #480 is probably a good way to go. But this is a good point that isn't handled anywhere else.
There was a problem hiding this comment.
All utcnow() are replaced and continue to produce naive datetime objects in UTC.
|
|
7b13071 to
dcd915a
Compare
Sigh 🤦🏻♂️ Consider that a magnificent brain fart on my side. |
|
@mmerickel what do you think of the above comments/questions, esp #475 (comment)? |
|
Thank you for this, and for the patience on the long review cycle here. There were three overlapping PRs open against #473 and #430 (this one, #480 and #491), all touching the same lines, so none could be merged alongside the others. I've combined them into #496, where your commit is preserved with you as the author. Your change to convert an aware One adjustment worth flagging: the For the deprecation itself #496 uses the centralized Closing here since the work continues in #496 — it's open for review, not merged yet. |
Combining #475 and #480 leaves one behavior change worth guarding: 475's unconditional v.astimezone(timezone.utc) treats a *naive* datetime as local time, shifting it by the machine's UTC offset. WebOb has always treated naive datetimes as UTC, and CI runs in UTC so the change is invisible there. Gate the conversion on v.tzinfo, keeping 475's fix for aware datetimes while leaving naive ones alone, and cover the naive/aware/date paths plus the new utcnow() helper.
3 participants
As per issue #473
I didn’t add tests yet, see my comments below.
Still need to format the code, but it looks like Windows tests fail because, I suspect, of this data source discrepancy.