This repository was archived by the owner on Jun 1, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 10
Timezone awareness #53
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5262958
Merge pull request #52 from IdentityPython/develop
peppelinux 4e79298
CryptoJWT version 1.6.0 doesn't exit yet.
rohe 7da1375
Merge branch 'master' of github.com:IdentityPython/JWTConnect-Python-…
rohe b63cf62
Updated version.
rohe 3b89449
fix: utcnow timestamp
peppelinux df56512
fix: cryptojwt up to 1.6.0
peppelinux 4dc5eec
fix: time_utils now are timezone aware and compatible with naive date…
peppelinux 40ae4c3
v1.5.3
peppelinux File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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