Fix UTC/local inconsistencies for naive datetimes #1506

plammens · 2019-09-04T14:53:38Z

Closes #1505.

(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.

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.

jh0ker

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

telegram/ext/jobqueue.py

jh0ker · 2019-10-11T20:32:55Z

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.

plammens · 2019-10-13T22:37:52Z

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 · 2019-10-13T22:41:53Z

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

tsnoam

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?

telegram/utils/helpers.py

telegram/ext/jobqueue.py

telegram/utils/helpers.py

telegram/ext/jobqueue.py

plammens · 2019-10-18T13:33:06Z

@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

Hi @plammens

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

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

telegram/utils/helpers.py

Conflicts: tests/test_bot.py

plammens · 2019-11-11T03:40:10Z

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

Sorry for the delay!

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.

tsnoam · 2019-11-15T20:44:31Z

@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 · 2019-11-15T20:51:01Z

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

plammens added 2 commits September 4, 2019 16:01

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 added the pending review label Sep 6, 2019

Poolitzer added this to the 12.2 milestone Sep 15, 2019

jh0ker reviewed Oct 11, 2019

View reviewed changes

telegram/ext/jobqueue.py Show resolved Hide resolved

telegram/ext/jobqueue.py Outdated Show resolved Hide resolved

jh0ker added pending reply and removed pending review labels Oct 11, 2019

fix: take timezone into account for checking weekday

56466ac

jh0ker added the pending review label Oct 13, 2019

jh0ker removed the pending reply label Oct 13, 2019

jh0ker requested a review from tsnoam October 13, 2019 22:39

tsnoam requested changes Oct 14, 2019

View reviewed changes

tsnoam added pending reply and removed pending review labels Oct 14, 2019

plammens added 3 commits October 18, 2019 13:23

refactor: fix style issues in python-telegram-bot#1506

6ed29bb

fix: signature breaking changes

7806071

fix: make absolute datetime and reference_timestamp mutually exclusive

cc40d63

tsnoam reviewed Oct 26, 2019

View reviewed changes

telegram/utils/helpers.py Outdated Show resolved Hide resolved

telegram/utils/helpers.py Outdated Show resolved Hide resolved

tsnoam and others added 3 commits November 9, 2019 21:15

Merge remote-tracking branch 'origin/master' into fix-datetimes

9eaef0f

Conflicts: tests/test_bot.py

Merge remote-tracking branch 'origin/master' into fix-datetimes

2fc93fe

fix: don't accept None in to_float_timestamp

82fe342

plammens force-pushed the fix-datetimes branch from 9da9dc1 to 5d11c26 Compare November 11, 2019 03:35

plammens added 2 commits November 11, 2019 11:49

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.

fix: naming of internal variables

faaed20

plammens force-pushed the fix-datetimes branch from 5d11c26 to f962c6d Compare November 11, 2019 20:13

plammens added 2 commits November 11, 2019 21:16

tests: more tests for time helpers & aware datetimes

3d3b707

fix: bug in merge 2fc93fe & 9eaef0f

1396ca9

plammens force-pushed the fix-datetimes branch from 0c29dce to 1396ca9 Compare November 11, 2019 21:17

cosmetic fixes

a2018fe

tsnoam approved these changes Nov 15, 2019

View reviewed changes

tsnoam merged commit 4e717a1 into python-telegram-bot:master Nov 15, 2019

Bibo-Joshi mentioned this pull request Nov 15, 2019

Add tz kwarg to from_timestamp() #1613

Closed

plammens mentioned this pull request Nov 16, 2019

Add last keyword argument to run_repeating #1497

Closed

Bibo-Joshi mentioned this pull request Nov 18, 2019

Add tz kwarg to from_timestamp() #1621

Merged

n5y mentioned this pull request Dec 8, 2019

Naive datetime should be treated as local timezone. UTC datetimes should be timezone aware. #1658

Closed

github-actions bot locked and limited conversation to collaborators Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UTC/local inconsistencies for naive datetimes #1506

Fix UTC/local inconsistencies for naive datetimes #1506

plammens commented Sep 4, 2019

jh0ker left a comment

jh0ker commented Oct 11, 2019

plammens commented Oct 13, 2019

jh0ker commented Oct 13, 2019

tsnoam left a comment

plammens commented Oct 18, 2019 •

edited

Loading

tsnoam left a comment

plammens commented Nov 11, 2019 •

edited

Loading

tsnoam commented Nov 15, 2019 •

edited

Loading

tsnoam commented Nov 15, 2019

Fix UTC/local inconsistencies for naive datetimes #1506

Fix UTC/local inconsistencies for naive datetimes #1506

Conversation

plammens commented Sep 4, 2019

jh0ker left a comment

Choose a reason for hiding this comment

jh0ker commented Oct 11, 2019

plammens commented Oct 13, 2019

jh0ker commented Oct 13, 2019

tsnoam left a comment

Choose a reason for hiding this comment

plammens commented Oct 18, 2019 • edited Loading

tsnoam left a comment

Choose a reason for hiding this comment

plammens commented Nov 11, 2019 • edited Loading

tsnoam commented Nov 15, 2019 • edited Loading

tsnoam commented Nov 15, 2019

plammens commented Oct 18, 2019 •

edited

Loading

plammens commented Nov 11, 2019 •

edited

Loading

tsnoam commented Nov 15, 2019 •

edited

Loading