Skip to content

Fix UTC/local inconsistencies for naive datetimes #1506

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
Merged
merged 14 commits into from
Nov 15, 2019
Merged

Fix UTC/local inconsistencies for naive datetimes #1506

merged 14 commits into from
Nov 15, 2019

Conversation

plammens
Copy link
Contributor

@plammens plammens commented Sep 4, 2019

Closes #1505.

@plammens
fix: unify naive datetime treatment to assume UTC
ab64d4e 
(cherry-picked from c6dd3d7.
I've included the refactoring mentioned in python-telegram-bot#1497 to facilitate the
change.)

There was inconsistent use of UTC vs local times. For instance, in the
former `_timestamp` helper (now `_datetime_to_float_timestamp`), assumed
that naive `datetime.datetime` objects were in the local timezone, while
the `from_timestamp` helper —which I would have thought was the
corresponding inverse function— returned naïve objects in UTC.

This meant that, for instance, `telegram.Message` objects' `date` field
was constructed as a naïve `datetime.datetime` (from the timestamp sent
by Telegram's server) in *UTC*, but when it was stored in `JSON` format
through the `to_json` method, the naïve `date` would be assumed to be in
*local time*, thus generating a different timestamp from the one it was
built from.

See python-telegram-bot#1505 for extended discussion.
@plammens
fix: more local --> UTC fixes for naive datetimes
cdf07b8 
Some tests/test fixtures that were using `datetime.datetime.now()` as
a test value, were changed to `datetime.datetime.utcnow()`, since now
everything is (hopefully) expecting UTC for naive datetimes.
@Bibo-Joshi Bibo-Joshi added the 📋 pending-review work status: pending-review label Sep 6, 2019
@Poolitzer Poolitzer added this to the 12.2 milestone Sep 15, 2019
jh0ker
jh0ker reviewed Oct 11, 2019
View reviewed changes
Copy link
Member

@jh0ker jh0ker left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. I left two comments, if you could address them that would be great.

@jh0ker jh0ker added 📋 pending-reply work status: pending-reply and removed 📋 pending-review work status: pending-review labels Oct 11, 2019
@jh0ker
Copy link
Member

jh0ker commented Oct 11, 2019

Some more questions that popped up during the discussion in the dev group. Are timezone-aware datetimes in the job queue supported with this? Were they supported before? There seem to be no tests for this yet.

@jh0ker jh0ker added the 📋 pending-review work status: pending-review label Oct 13, 2019
@plammens
Copy link
Contributor Author

Are timezone-aware datetimes in the job queue supported with this?

Yes; namely, when in run_once, first in run_repeating, and time in run_daily now all accept timezone-aware objects (and use that information adequately). It all boils down to _datetime_to_float_timestamp, which now supports aware datetimes, so I believe everything else in the repository that handles date/time objects by converting them to timestamps also supports aware datetimes now.

https://github.com/plammens/python-telegram-bot/blob/56466ac449028e4c5eb0470c919ec2b7466d6737/telegram/utils/helpers.py#L58-L61

Were they supported before?

No; for example you would get a TypeError when passing an aware datetime to when in run_once.

There seem to be no tests for this yet.

Will add in the near future.

@jh0ker jh0ker removed the 📋 pending-reply work status: pending-reply label Oct 13, 2019
@jh0ker jh0ker requested a review from tsnoam October 13, 2019 22:39
@jh0ker
Copy link
Member

jh0ker commented Oct 13, 2019

Thank you very much! This looks very nice to me, but timezones confuse me so i requested a review from @tsnoam 😉

tsnoam
tsnoam requested changes Oct 14, 2019
View reviewed changes
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

All in all, this is really great. Thank you!

However, I have several comments / changes needs to be done.
Will you be so kind to check them out?

@tsnoam tsnoam added 📋 pending-reply work status: pending-reply and removed 📋 pending-review work status: pending-review labels Oct 14, 2019
@plammens
Copy link
Contributor Author

plammens commented Oct 18, 2019 •
edited
Loading

@tsnoam Before merging I think I should add:

  • Tests to check that timezone aware datetimes work
  • A general note somewhere (where?) in the documentation stating that all naive datetimes will be assumed to be in UTC
  • A from_float_timestamp for symmetry? Or rename from_timestamp to maybe_from_timestamp to indicate that is just a wrapper over datetime.datetime.fromtimestamp?
  • Add capability for aware datetimes to from_timestamp?
  • Restrict to_float_timestamp to non-None times

Anything else/any thoughts?

tsnoam
tsnoam reviewed Oct 26, 2019
View reviewed changes
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

Hi @plammens

This looks very good. Almost ready for merge :-)

  1. About the tests: yes, we need tests... also the tests @jh0ker had suggested in his review.
  2. I don't think any extra documentation is required for now. The docstring of to_float_timestamp is good enough.
  3. Re: renaming funciton names - see the comments on the review notes. But basically, leave it as is.
  4. Still need to fix JobQueue._put() so it won't pass None to to_float_timestamp().
  5. Adding similar capabilities to from_timestamp() can be done in a different PR. Lets wrap this one and merge it already ;-)

@plammens
Copy link
Contributor Author

plammens commented Nov 11, 2019 •
edited
Loading

@tsnoam I've addressed the remaining points and added tests.

Sorry for the delay!

@plammens
fix: raise error in JobQueue._put if no time specification is given
1ae2650 
A job shouldn't (and can't) be enqueued with `next_t = None`. An
exception should be raised at `_put` before an obscure error occurs
later down the line.
@plammens
fix: naming of internal variables
faaed20
@tsnoam
Copy link
Member

tsnoam commented Nov 15, 2019 •
edited
Loading

@plammens
Thank you very much for your contribution. Also your patience for us to reply and with all our requests is highly appreciated.
I've pushed a minor cosmetic fix, and now we're just waiting for unitests to pass (no reason they shouldn't) before merging.

@tsnoam
Copy link
Member

tsnoam commented Nov 15, 2019

I don't know why pre-commit test failed. On my machine it passes successfully.
Merging anyway.

@tsnoam tsnoam merged commit 4e717a1 into python-telegram-bot:master Nov 15, 2019
@Bibo-Joshi Bibo-Joshi mentioned this pull request Nov 15, 2019
Closed
@plammens plammens mentioned this pull request Nov 16, 2019
Closed
@Bibo-Joshi Bibo-Joshi mentioned this pull request Nov 18, 2019
Merged
@n5y n5y mentioned this pull request Dec 8, 2019
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jh0ker jh0ker jh0ker left review comments

@tsnoam tsnoam tsnoam approved these changes

Assignees
No one assigned
Labels
📋 pending-reply work status: pending-reply
Projects
None yet
Milestone
12.3
Development

Successfully merging this pull request may close these issues.

Inconsistent use of UTC/local naïve datetimes
5 participants
@plammens @jh0ker @tsnoam @Bibo-Joshi @Poolitzer