Skip to content

Conversation

jenstroeger
Copy link

@jenstroeger jenstroeger commented Dec 12, 2024

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.

Copy link
Author

@jenstroeger jenstroeger left a comment

Choose a reason for hiding this comment

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

A few comments below to continue the conversation from issue #473.

Comment on lines 242 to 243
v = datetime.utcnow() + v
v = datetime.now(tz=zoneinfo.ZoneInfo("UTC")) + v
Copy link
Author

Choose a reason for hiding this comment

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

The string formatting below doesn’t care that the datetime object is aware here.

Comment on lines 245 to 247
if isinstance(v, datetime):
v = v.astimezone(zoneinfo.ZoneInfo("UTC")).timetuple()
elif isinstance(v, date):
v = v.timetuple()
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

All utcnow() are replaced and continue to produce naive datetime objects in UTC.

@mmerickel
Copy link
Member

datetime.timezone.utc was added in python 3.2 - is there a reason not to use it in favor of zoneinfo?

@jenstroeger jenstroeger force-pushed the replace-naive-utcnow-with-aware-now branch from 7b13071 to dcd915a Compare December 13, 2024 00:51
@jenstroeger
Copy link
Author

datetime.timezone.utc was added in python 3.2 - is there a reason not to use it in favor of zoneinfo?

Sigh 🤦🏻‍♂️ Consider that a magnificent brain fart on my side.

Copy link
Member

@mmerickel mmerickel left a comment

Choose a reason for hiding this comment

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

LGTM

@jenstroeger
Copy link
Author

jenstroeger commented Dec 13, 2024

@mmerickel what do you think of the above comments/questions, esp #475 (comment)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants