Skip to content

Fix UTC/local inconsistencies for naive datetimes#1506

Merged
tsnoam merged 14 commits intopython-telegram-bot:masterfrom
plammens:fix-datetimes
Nov 15, 2019
Merged

Fix UTC/local inconsistencies for naive datetimes#1506
tsnoam merged 14 commits intopython-telegram-bot:masterfrom
plammens:fix-datetimes

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

plammens commented Oct 13, 2019

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