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

Refactor datetime.utcnow() to datetime.now(timezone.utc) #80

Conversation

mithun2003
Copy link

@mithun2003 mithun2003 commented Dec 16, 2023

Updated datetime.utcnow() to datetime.now(timezone.utc) in multiple modules and added timezone.utc

Solve this issue:
#79

Updated datetime.utcnow() to datetime.now(timezone.utc) in multiple
modules and added timezone.utc to ensure consistent handling of time
across the application.
@mithun2003 mithun2003 closed this Dec 16, 2023
@mithun2003 mithun2003 reopened this Dec 16, 2023
@mithun2003 mithun2003 changed the base branch from main to 79-datetimeutcnow-is-deprecated-as-of-python-312 December 16, 2023 09:46
@igorbenav igorbenav self-requested a review December 17, 2023 01:48
@igorbenav igorbenav added the enhancement New feature or request label Dec 17, 2023
@igorbenav igorbenav self-assigned this Dec 17, 2023
Copy link
Owner

@igorbenav igorbenav left a comment

Choose a reason for hiding this comment

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

1. Tests are not passing

I'm getting:

error = DataError("invalid input for query argument $1: datetime.datetime(2023, 12, 17, 1, 57, 1... (can't subtract offset-naive and offset-aware datetimes)")

Probably because of access token stuff, adding datetime.now + expires_delta.

Also:

asyncpg.exceptions.DataError: invalid input for query argument $1: datetime.datetime(2023, 12, 17, 1, 57, 1... (can't subtract offset-naive and offset-aware datetimes)

You need to handle saving timestamp aware datetime in db. This would be done adding timezone=True to the column definition in the sqlalchemy model, like this:

created_at = Column(DateTime(timezone=True), default=lambda: datetime.now(UTC))

2. Imports update

You may change all timezone.utc uses, like:

from datetime import timezone
...

current_timestamp = int(datetime.now(timezone.utc).timestamp())

With UTC, like this:

from datetime import UTC
...

current_timestamp = int(datetime.now(UTC).timestamp())

This way is simpler and works as of python 3.11

Mithun Thomas added 3 commits December 18, 2023 09:00
the application

Update datetime handling across the application to consistently utilize
UTC, ensuring uniformity and eliminating potential discrepancies due to
time zone differences.
awareness

Updated the datetime usage in multiple models to include timezone
awareness by importing 'UTC' and applying it to the default_factory.
@mithun2003
Copy link
Author

Solve all the mentioned bugs and change timezone.utc to UTC

Copy link
Owner

@igorbenav igorbenav left a comment

Choose a reason for hiding this comment

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

Tests still not passing.
I think the only part that is not working now is, in all model definitions, change this:

updated_at: Mapped[Optional[datetime]] = mapped_column(default=None)
deleted_at: Mapped[Optional[datetime]] = mapped_column(default=None)

With

updated_at: Mapped[Optional[datetime]] = mapped_column(DateTime(timezone=True), default=None)
deleted_at: Mapped[Optional[datetime]] = mapped_column(DateTime(timezone=True), default=None)

To create the columns as timestamp-aware. With the way things are now, they are create as timestamp-naive.

Try running the tests define in docker-compose.yml

@mithun2003
Copy link
Author

I solve that issue too and do the testing. And pass all the test cases
Screenshot 2023-12-27 124000

@igorbenav
Copy link
Owner

Nice one, @mithun2003, thanks for the contribution!🚀

@igorbenav igorbenav merged commit 11786ad into igorbenav:79-datetimeutcnow-is-deprecated-as-of-python-312 Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants