Skip to content
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

Replace all uses of utcnow with tz-aware datetimes #72

Merged
merged 3 commits into from
Mar 3, 2025

Conversation

brandon-doist
Copy link
Contributor

Needed so we emit no warnings when running on python 3.12 systems.

AFAICT, these datetimes are mainly just used to determine redis keys, where the naive/aware makes no difference, and the previous "naive" datetimes were all implicitly UTC anyways (via utcnow).

However, this does change period_start and period_end to return aware datetimes, which is technically breaking!

If you folks notice anything beyond that, please let me know and I'll take a look 🙂

Choose a reason for hiding this comment

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

PR Overview

This PR updates all datetime usages to be timezone‐aware, replacing instances of datetime.utcnow() with datetime.now(tz=timezone.utc) to prevent warnings on Python 3.12 systems. Key changes include:

  • Adding timezone.utc to datetime calls in tests and production code.
  • Adjusting datetime.strptime results to be tz-aware.
  • Updating documentation and test examples to use tz-aware datetimes.

Reviewed Changes

File Description
test/test_cohort.py Updated datetime instances to be tz-aware in fixture definitions.
test/test_bitmapist.py Replaced utcnow() with now(tz=timezone.utc) in multiple test cases.
bitmapist/cohort/init.py Made start_date conversion tz-aware by using .replace(tzinfo=timezone.utc).
bitmapist/init.py Consistently updated datetime calls and type annotations to use tz-aware datetimes.
test/test_period_start_end.py Modified test instances to include a timezone in the datetime constructors.
test/conftest.py Updated deprecated pytest.yield_fixture to pytest.fixture.
test/test_from_date.py Adjusted test cases to pass tz-aware datetimes to event constructors.
README.md Updated examples to demonstrate creating tz-aware datetime objects.

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Copy link
Member

@goncalossilva goncalossilva left a comment

Choose a reason for hiding this comment

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

Changes look great, and tests pass!

I'm not a bitmapist expert but it's not clear to me that the change to period_start and period_end have a practical impact, considering they were naive but running on a machine whose tz is most likely set to UTC. I may be missing something, but I think the result is the same. :)

@brandon-doist
Copy link
Contributor Author

brandon-doist commented Feb 28, 2025

I'm not a bitmapist expert but it's not clear to me that the change to period_start and period_end have a practical impact, considering they were naive but running on a machine whose tz is most likely set to UTC. I may be missing something, but I think the result is the same. :)

@goncalossilva Yeah, in terms of todoist, I agree with you! I don't even think we reference period_start or period_end anywhere in our code. But this is a public library, so in theory others (with different server configurations) might be, so I thought it'd be worth pointing out still

(it'll be released under a new version, right, so it's not like we're going to surprise-break things on anyone, I hope)

@brandon-doist brandon-doist merged commit dac0d60 into main Mar 3, 2025
4 checks passed
@brandon-doist brandon-doist deleted the brandon/312 branch March 3, 2025 16:14
Copy link
Member

@amix amix left a comment

Choose a reason for hiding this comment

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

The changes look good from my POV. Thanks, @brandon-doist 👏

@brandon-doist
Copy link
Contributor Author

TY for the review @amix !

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.

3 participants