Add last kwarg to run_repeating#1345
Closed
plammens wants to merge 6 commits intopython-telegram-bot:V12from
Closed
Add last kwarg to run_repeating#1345plammens wants to merge 6 commits intopython-telegram-bot:V12from
last kwarg to run_repeating#1345plammens wants to merge 6 commits intopython-telegram-bot:V12from
Conversation
The procedure to convert a given time object to a float containing the "seconds until the next run" was extracted into an auxiliary top-level function named `_to_interval_seconds`. Also, `_put`'s "prep phase" was refactored as follows. A new local identifier named `interval` was created to make its meaning clearer (instead of always reusing `next_t`, which was confusing). The way in which the interval is fetched (and checked for `None`) has also been simplified. Finally, `next_t`'s last assignment is the actual timestamp in which the job is to be run, which is either `previous_t` (if available) or the current time plus the amount stored in `interval`. Finally, `last_t` was renamed to `previous_t` to avoid confusion with upcoming `last` kwarg.
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.
Instead of converting to interval first and then adding the current (or previous) time.
Member
|
Hi, Thanks for your contribution. As you can see tests fail on py2. This is because you modified the calculation for Pieter |
Contributor
Author
|
Ah! Sorry I missed that. Will fix. By the way, could time objects be stored as |
Python 2's datetime module doesn't have the datetime.timestamp() method. An equivalent calculation has been put in its place.
Contributor
Author
|
@Eldinnie Should be fixed now. Though Travis keeps failing on the unrelated |
Contributor
Author
|
@Eldinnie Any updates on this? I can make changes if the PR is not up to scratch |
Member
|
This was closed when the V12 branch was deleted. Would you mind reopening the PR to master, @plammens ? :) |
Contributor
Author
|
@Bibo-Joshi Oh, that makes sense. Reopened as #1497! |
Member
|
@plammens Thanks! |
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Added a
lastkeyword argument to therun_repeatingmethod of job queues that takes in a time specifier (in the same formats supported byfirst). It specifies the last time the job should be run. Currently, checking if the job should be run or not involves a certain "delay buffer" (so that iflastis an exact multiple ofinterval, the internal delays don't mess up the intended behaviour), but it's a temporary solution. Also, I've made some minor refactorings.Tests run correctly from my side. I've added one test for the new feature, but maybe a few more would be in place. The docs haven't been updated yet.
Closes #1333.