Add last keyword argument to run_repeating#1497
Add last keyword argument to run_repeating#1497plammens wants to merge 3 commits intopython-telegram-bot:masterfrom
last keyword argument to run_repeating#1497Conversation
There was a problem hiding this comment.
Thanks for this PR @plammens!
This surely is a nice feature to have and the code looks good to me. Two thoughts:
- I feel like
_to_timestampcould be moved totelegram.utils.helpers. What do the other @python-telegram-bot/developers think? - Maybe add tests for the now
_to_timestamp, especially if it's moved
And docs should be added, of course
5a323d6 to
f26f400
Compare
|
@Bibo-Joshi
I realised that rambling on about the changes I've made without showing them is a bit useless. Tomorrow I'll push what I've got (we'll roll back if necessary). In any case, what do you think about all this? |
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#1497 for extended discussion.
|
Thank for your detailed report, @plammens ! About |
(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.
|
About 5.: We're looking to drop Python 2 support in version 13 (see #1538). Perhaps it makes sense to wait for that? |
|
@plammens |
Added a `last` kwarg to `run_repeating` that specifies when the job should last be run. The checking is done before executing the job, but with a small delay buffer, so that if `last` is an exact multiple of `interval`, the last job is still run, even though the actual time is slightly after the theoretical finish time.
Add parametrisations, extract constants and helpers.
3217b96 to
76be57a
Compare
|
@tsnoam I've rebased this using the stuff introduced in #1506; I've also squashed a little. I think now the only thing to discuss is 4. (comment above). Do you agree that it should be a strict check? And thus |
|
TL;DR: Closing in favor of #1981 After internal discussion, we came to the conclusion that we don't want to continue implementing scheduling logic on our own and instead rely on a 3rd party library for that (see #1936, #1981). |
See #1345 (reopened on
masterinstead ofV12):Closes #1333.