Skip to content
This repository was archived by the owner on Jun 1, 2023. It is now read-only.

Timezone awareness #53

Merged
merged 8 commits into from
Nov 18, 2021
Merged

Timezone awareness #53

merged 8 commits into from
Nov 18, 2021

Conversation

peppelinux
Copy link
Member

this PR fixed the datetime utcnow timestamp that still returned a non utc timestamp

@rohe
Copy link
Collaborator

rohe commented Nov 17, 2021

As you commented in another communication. CryptoJWT 1.6.0 is indeed on PyPI.
So we should revert that change.

@peppelinux
Copy link
Member Author

As you commented in another communication. CryptoJWT 1.6.0 is indeed on PyPI. So we should revert that change.

fixed here
df56512

we just have to squash before merging

@@ -200,7 +202,8 @@ def time_a_while_ago(
:return: datetime instance using UTC time
"""
delta = timedelta(days, seconds, microseconds, milliseconds, minutes, hours, weeks)
return datetime.utcnow() - delta
res = datetime.now(timezone.utc) - delta
return res.replace(tzinfo=None)
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to strip the timezone information?

Copy link
Member Author

Choose a reason for hiding this comment

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

naive and aware datetime can't be compared, so for retrocompatibility I operate with aware datetime but always return naive datetime

Copy link
Member Author

Choose a reason for hiding this comment

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

considering that standard requries utc datetime, it's not important to have it with timezone

Copy link
Member

Choose a reason for hiding this comment

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

What are those comparisons?

It is important it to have it with timezone to ensure that operations on time are done in the needed timezone.
I would argue we need to do the opposite - have all datetimes in UTC to ensure calculations are correct. So, the comparisons that take place, should also have dates loaded as UTC.

Copy link
Member Author

Choose a reason for hiding this comment

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

the comparison doesn't happen here but later in a unit test

______________________________________________________________________________________________ test_time_a_while_ago _______________________________________________________________________________________________

    def test_time_a_while_ago():
        dt = datetime.utcnow()
        t = time_a_while_ago(seconds=10)
>       delta = dt - t  # slightly less than 10
E       TypeError: can't subtract offset-naive and offset-aware datetimes

tests/test_03_time_util.py:204: TypeError

considering that standard require UTC timestamp I didn't see any needs to carrying aware datetimes in the code BUT you kow that I'ma good boy that can spend some more effort to get all happy :)

Roland approved, do we have to put another change or do you think that's good enough as is?

Copy link
Member

Choose a reason for hiding this comment

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

If we are going to change .utcnow() to .now(utc) I think we should do it everywhere.
If we don't want to do it, then let's not do it at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know the conseguences to other libraries and behaviours
I preferred to let the type be the same as before to prevent any breaking changes with this release

that's the answer, feel free to open an issue to keep track of this

@peppelinux
Copy link
Member Author

@rohe @c00kiemon5ter can we merge this as it is or do I have to return a tz aware datetime and fix the unit test as well?

@c00kiemon5ter
Copy link
Member

@rohe @c00kiemon5ter can we merge this as it is or do I have to return a tz aware datetime and fix the unit test as well?

go forward!

@rohe
Copy link
Collaborator

rohe commented Nov 18, 2021

Absolutely, merge it !

@peppelinux peppelinux changed the title Timezone awarness Timezone awareness Nov 18, 2021
@peppelinux
Copy link
Member Author

Ok guys, just to tell that the only other place where utcnow is used is here

ltz = datetime.datetime.utcnow().astimezone().tzinfo

and that's just to get the TZINFO from the host

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants