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.

Sign up for GitHub

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

Jump to bottom

Replace datetime.utcfromtimestamp #404

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Replace datetime.utcfromtimestamp #404

wants to merge 1 commit into from
+26 −16

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Jan 4, 2025 •
edited
Loading

It is depricated and to be removed soon.
It also makes warning to pop up:

  python-driver/cassandra/cqlengine/columns.py:554: DeprecationWarning: datetime.datetime.utcfromtimestamp() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.fromtimestamp(timestamp, datetime.UTC).
    return datetime.utcfromtimestamp(value)

Only method available out-of-box for this purpose is datetime.fromtimestamp.
But it either include timezone into datetime which is not acceptable (because datetimeimplementation does not allow you to comparedatetimewithtz_infoand wihtout it) or offsetdatetimefrom utc which we don't want to happen because it changes actual time. On other handtz_infoin not writable ondatetimeand all API that allows you to manipulate it offsets time as well. So, simpliest solution would to be ship a function that callsdatetime.fromtimestampand then builds exactly samedatetimewithtz_info` empty.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@dkropachev dkropachev force-pushed the dk/replace-utcfromtimestamp branch 2 times, most recently from caaee9f to 7994dd2 Compare January 5, 2025 02:57
@dkropachev dkropachev changed the title Replace datetime.utcfromtimestamp by datetime.fromtimestamp Replace datetime.utcfromtimestamp Jan 5, 2025
Lorak-mmk
Lorak-mmk requested changes Jan 5, 2025
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

After reading PR description and the commit message it is still not obvious to me why is this change done this way. I assume that the method is deprecated for a reason. I don't understand why we basically ignore the deprecation and recreate the behavior of this method instead of using the solution from the deprecation warning.

In the commit message you wrote "either include timezone into datetime, which we
do not want, or offset datetime from utc.", but you did not explain why we do not want it.

I'm not saying the patch is wrong (it is probably good), but I need more detailed explanation - it will be useful for future readers.

cassandra/query.py Outdated
Comment on lines 28 to 31
from cassandra import ConsistencyLevel, OperationTimedOut
from cassandra.util import unix_time_from_uuid1, maybe_add_timeout_to_query
from cassandra.util import unix_time_from_uuid1, maybe_add_timeout_to_query, utcfromtimestamp
from cassandra.encoder import Encoder
import cassandra.encoder
Copy link

Choose a reason for hiding this comment

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

❓ I see that you added this import in this file, but did not change anything else in the file. Is the import used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@dkropachev
Replace datetime.utcfromtimestamp
18e2644 
It is depricated and to be removed soon.
Only method available out-of-box for this purpose is `datetime.fromtimestamp`.
But it either include timezone into datetime which is not acceptable, or offset datetime from utc which we don't want to happen.
On other hand `tz_info` in not writable on `datetime` and all API that
allows you to manipulate it offsets time.
So, simpliest solution would to be ship a function that calls `datetime.fromtimestamp` and then builds exactly same datetime with tz_info empty.
@dkropachev dkropachev force-pushed the dk/replace-utcfromtimestamp branch from 7994dd2 to 18e2644 Compare January 5, 2025 13:18
@dkropachev dkropachev requested a review from Lorak-mmk January 5, 2025 13:19
@dkropachev
Copy link
Collaborator Author

After reading PR description and the commit message it is still not obvious to me why is this change done this way. I assume that the method is deprecated for a reason. I don't understand why we basically ignore the deprecation and recreate the behavior of this method instead of using the solution from the deprecation warning.

In the commit message you wrote "either include timezone into datetime, which we do not want, or offset datetime from utc.", but you did not explain why we do not want it.

I'm not saying the patch is wrong (it is probably good), but I need more detailed explanation - it will be useful for future readers.

After reading PR description and the commit message it is still not obvious to me why is this change done this way. I assume that the method is deprecated for a reason. I don't understand why we basically ignore the deprecation and recreate the behavior of this method instead of using the solution from the deprecation warning.

In the commit message you wrote "either include timezone into datetime, which we do not want, or offset datetime from utc.", but you did not explain why we do not want it.

I'm not saying the patch is wrong (it is probably good), but I need more detailed explanation - it will be useful for future readers.

Please take a look again

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

@sylwiaszunejko sylwiaszunejko Awaiting requested review from sylwiaszunejko

@Lorak-mmk Lorak-mmk Awaiting requested review from Lorak-mmk

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dkropachev @Lorak-mmk