From d0d15e618b38b197b568646de18f34d39281172a Mon Sep 17 00:00:00 2001 From: Paolo Lammens Date: Thu, 14 Feb 2019 19:07:30 +0100 Subject: [PATCH 1/6] refactor: Extract conversion to interval seconds from various types 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. --- telegram/ext/jobqueue.py | 57 ++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/telegram/ext/jobqueue.py b/telegram/ext/jobqueue.py index e3cfacfd9ae..07249911d5b 100644 --- a/telegram/ext/jobqueue.py +++ b/telegram/ext/jobqueue.py @@ -70,27 +70,21 @@ def __init__(self): def set_dispatcher(self, dispatcher): self._dispatcher = dispatcher - def _put(self, job, next_t=None, last_t=None): - if next_t is None: - next_t = job.interval - if next_t is None: - raise ValueError('next_t is None') - - if isinstance(next_t, datetime.datetime): - next_t = (next_t - datetime.datetime.now()).total_seconds() - - elif isinstance(next_t, datetime.time): - next_datetime = datetime.datetime.combine(datetime.date.today(), next_t) - - if datetime.datetime.now().time() > next_t: - next_datetime += datetime.timedelta(days=1) - - next_t = (next_datetime - datetime.datetime.now()).total_seconds() + def _put(self, job, next_t=None, previous_t=None): + # """ + # Enqueues the job, scheduling its next run at ` + `. + # `` is the previous time the job was run, stored in `previous_t`, + # and defaults to `None`. + # `` is a time interval in seconds, and is calculated from the + # `next_t` kwarg (if given) or the job's interval. + # """ - elif isinstance(next_t, datetime.timedelta): - next_t = next_t.total_seconds() + # get time distance in seconds till run: + interval = _to_interval_seconds(next_t or job.interval) + if interval is None: + raise ValueError('interval is None') - next_t += last_t or time.time() + next_t = (previous_t or time.time()) + interval # time at which to run self.logger.debug('Putting job %s with t=%f', job.name, next_t) @@ -266,7 +260,7 @@ def tick(self): self.logger.debug('Skipping disabled job %s', job.name) if job.repeat and not job.removed: - self._put(job, last_t=t) + self._put(job, previous_t=t) else: self.logger.debug('Dropping non-repeating or removed job %s', job.name) @@ -488,3 +482,26 @@ def job_queue(self, job_queue): def __lt__(self, other): return False + + +def _to_interval_seconds(t): + # """ + # Converts a given time object (i.e., `datetime.datetime`, + # `datetime.time`, or `datetime.timedelta`) to seconds + # as a floating point number (if it isn't already). Used + # for converting given kwargs like `first` to a uniform format. + # """ + + if isinstance(t, datetime.datetime): + return (t - datetime.datetime.now()).total_seconds() + elif isinstance(t, datetime.time): + next_datetime = datetime.datetime.combine(datetime.date.today(), t) + + if datetime.datetime.now().time() > t: + next_datetime += datetime.timedelta(days=1) + + return (next_datetime - datetime.datetime.now()).total_seconds() + elif isinstance(t, datetime.timedelta): + return t.total_seconds() + + return t From dc3e2feb620e456ddb1fab56b3ef4b96d67962c3 Mon Sep 17 00:00:00 2001 From: Paolo Lammens Date: Thu, 14 Feb 2019 20:17:42 +0100 Subject: [PATCH 2/6] feat: Add `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. --- telegram/ext/jobqueue.py | 44 +++++++++++++++++++++++++++++----------- tests/test_jobqueue.py | 7 +++++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/telegram/ext/jobqueue.py b/telegram/ext/jobqueue.py index 07249911d5b..0a204f2aa0e 100644 --- a/telegram/ext/jobqueue.py +++ b/telegram/ext/jobqueue.py @@ -130,7 +130,7 @@ def run_once(self, callback, when, context=None, name=None): self._put(job, next_t=when) return job - def run_repeating(self, callback, interval, first=None, context=None, name=None): + def run_repeating(self, callback, interval, first=None, last=None, context=None, name=None): """Creates a new ``Job`` that runs at specified intervals and adds it to the queue. Args: @@ -157,6 +157,11 @@ def run_repeating(self, callback, interval, first=None, context=None, name=None) tomorrow. Defaults to ``interval`` + last (:obj:`int` | :obj:`float` | :obj:`datetime.timedelta` | \ + :obj:`datetime.datetime` | :obj:`datetime.time`, optional): + Time in or at which the job should stop running. + This parameter will be interpreted depending on its type (same as for `first`). + If `None`, the job will run indefinitely. context (:obj:`object`, optional): Additional data needed for the callback function. Can be accessed through ``job.context`` in the callback. Defaults to ``None``. name (:obj:`str`, optional): The name of the new job. Defaults to @@ -170,6 +175,7 @@ def run_repeating(self, callback, interval, first=None, context=None, name=None) job = Job(callback, interval=interval, repeat=True, + finish_time=last, context=context, name=name, job_queue=self) @@ -242,6 +248,9 @@ def tick(self): self._set_next_peek(t) break + delay_buffer = 0.01 + if job.finish_time is not None and now > job.finish_time + delay_buffer: + job.schedule_removal() # job shouldn't run anymore if job.removed: self.logger.debug('Removing job %s', job.name) continue @@ -262,7 +271,7 @@ def tick(self): if job.repeat and not job.removed: self._put(job, previous_t=t) else: - self.logger.debug('Dropping non-repeating or removed job %s', job.name) + self.logger.debug('Dropping non-repeating, removed, or finished job %s', job.name) def start(self): """Starts the job_queue thread.""" @@ -353,6 +362,7 @@ def __init__(self, callback, interval=None, repeat=True, + finish_time=None, context=None, days=Days.EVERY_DAY, name=None, @@ -362,10 +372,12 @@ def __init__(self, self.context = context self.name = name or callback.__name__ - self._repeat = repeat + self._repeat = None self._interval = None + self._finish_time = None self.interval = interval self.repeat = repeat + self.finish_time = finish_time self._days = None self.days = days @@ -448,6 +460,14 @@ def repeat(self, repeat): raise ValueError("'repeat' can not be set to 'True' when no 'interval' is set") self._repeat = repeat + @property + def finish_time(self): + return self._finish_time + + @finish_time.setter + def finish_time(self, time_): + self._finish_time = time.time() + _to_interval_seconds(time_) + @property def days(self): """Tuple[:obj:`int`]: Optional. Defines on which days of the week the job should run.""" @@ -484,7 +504,7 @@ def __lt__(self, other): return False -def _to_interval_seconds(t): +def _to_interval_seconds(time_): # """ # Converts a given time object (i.e., `datetime.datetime`, # `datetime.time`, or `datetime.timedelta`) to seconds @@ -492,16 +512,16 @@ def _to_interval_seconds(t): # for converting given kwargs like `first` to a uniform format. # """ - if isinstance(t, datetime.datetime): - return (t - datetime.datetime.now()).total_seconds() - elif isinstance(t, datetime.time): - next_datetime = datetime.datetime.combine(datetime.date.today(), t) + if isinstance(time_, datetime.datetime): + return (time_ - datetime.datetime.now()).total_seconds() + elif isinstance(time_, datetime.time): + next_datetime = datetime.datetime.combine(datetime.date.today(), time_) - if datetime.datetime.now().time() > t: + if datetime.datetime.now().time() > time_: next_datetime += datetime.timedelta(days=1) return (next_datetime - datetime.datetime.now()).total_seconds() - elif isinstance(t, datetime.timedelta): - return t.total_seconds() + elif isinstance(time_, datetime.timedelta): + return time_.total_seconds() - return t + return time_ diff --git a/tests/test_jobqueue.py b/tests/test_jobqueue.py index f0c7ac4051c..f4422d90ac7 100644 --- a/tests/test_jobqueue.py +++ b/tests/test_jobqueue.py @@ -98,6 +98,13 @@ def test_run_repeating_first(self, job_queue): sleep(0.07) assert self.result == 1 + def test_run_repeating_last(self, job_queue): + job_queue.run_repeating(self.job_run_once, 0.05, last=0.1) + sleep(0.12) + assert self.result == 2 + sleep(0.1) + assert self.result == 2 + def test_multiple(self, job_queue): job_queue.run_once(self.job_run_once, 0.01) job_queue.run_once(self.job_run_once, 0.02) From 32c9fe795d16a3286e78c1357b5d9b0fd9ddcf1a Mon Sep 17 00:00:00 2001 From: Paolo Lammens Date: Thu, 14 Feb 2019 22:18:47 +0100 Subject: [PATCH 3/6] =?UTF-8?q?fix:=20Ignore=20`None`=20=C2=B4finish=5Ftim?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- telegram/ext/jobqueue.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/telegram/ext/jobqueue.py b/telegram/ext/jobqueue.py index 0a204f2aa0e..970a2265326 100644 --- a/telegram/ext/jobqueue.py +++ b/telegram/ext/jobqueue.py @@ -248,7 +248,7 @@ def tick(self): self._set_next_peek(t) break - delay_buffer = 0.01 + delay_buffer = 0.01 # tolerance for last if job.finish_time is not None and now > job.finish_time + delay_buffer: job.schedule_removal() # job shouldn't run anymore if job.removed: @@ -462,11 +462,15 @@ def repeat(self, repeat): @property def finish_time(self): + """:obj:`float`: Optional. Time at which the job should stop repeating.""" return self._finish_time @finish_time.setter def finish_time(self, time_): - self._finish_time = time.time() + _to_interval_seconds(time_) + if time_ is not None: + if not self.repeat: + raise ValueError("'finish_time' cannot be set if job doesn't repeat") + self._finish_time = time.time() + _to_interval_seconds(time_) @property def days(self): From d039d6ef6f82155c5cb6d07c7ea979ab44915670 Mon Sep 17 00:00:00 2001 From: Paolo Lammens Date: Thu, 14 Feb 2019 23:03:52 +0100 Subject: [PATCH 4/6] refactor: Convert to timestamp directly Instead of converting to interval first and then adding the current (or previous) time. --- telegram/ext/jobqueue.py | 46 +++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/telegram/ext/jobqueue.py b/telegram/ext/jobqueue.py index 970a2265326..fc930c73cbd 100644 --- a/telegram/ext/jobqueue.py +++ b/telegram/ext/jobqueue.py @@ -79,12 +79,10 @@ def _put(self, job, next_t=None, previous_t=None): # `next_t` kwarg (if given) or the job's interval. # """ - # get time distance in seconds till run: - interval = _to_interval_seconds(next_t or job.interval) - if interval is None: - raise ValueError('interval is None') - - next_t = (previous_t or time.time()) + interval # time at which to run + # get time at which to run: + next_t = _to_timestamp(next_t or job.interval, previous_t=previous_t) + if next_t is None: + raise ValueError("'next_t' is None'") self.logger.debug('Putting job %s with t=%f', job.name, next_t) @@ -470,7 +468,9 @@ def finish_time(self, time_): if time_ is not None: if not self.repeat: raise ValueError("'finish_time' cannot be set if job doesn't repeat") - self._finish_time = time.time() + _to_interval_seconds(time_) + self._finish_time = _to_timestamp(time_) + else: + self._finish_time = None @property def days(self): @@ -508,24 +508,30 @@ def __lt__(self, other): return False -def _to_interval_seconds(time_): +def _to_timestamp(time_, previous_t=None): # """ # Converts a given time object (i.e., `datetime.datetime`, - # `datetime.time`, or `datetime.timedelta`) to seconds - # as a floating point number (if it isn't already). Used - # for converting given kwargs like `first` to a uniform format. + # `datetime.time`, `datetime.timedelta`, interval from now + # in seconds) to POSIX timestamp. Used for converting given + # kwargs like `first` to a uniform format. # """ - if isinstance(time_, datetime.datetime): - return (time_ - datetime.datetime.now()).total_seconds() + previous_t = previous_t or time.time() + + if time_ is None: + return None + elif hasattr(time_, 'timestamp'): + return time_.timestamp() elif isinstance(time_, datetime.time): - next_datetime = datetime.datetime.combine(datetime.date.today(), time_) + date = datetime.date.today() - if datetime.datetime.now().time() > time_: - next_datetime += datetime.timedelta(days=1) + if time_ < datetime.datetime.today().time(): + date += datetime.timedelta(days=1) - return (next_datetime - datetime.datetime.now()).total_seconds() + return datetime.datetime.combine(date, time_).timestamp() elif isinstance(time_, datetime.timedelta): - return time_.total_seconds() - - return time_ + return previous_t + time_.total_seconds() + elif isinstance(time_, Number): + return previous_t + time_ + else: + raise TypeError('Unable to convert to timestamp') From 0b2174e07a51376377094f17b1e49e266813b21f Mon Sep 17 00:00:00 2001 From: Paolo Lammens Date: Sun, 17 Feb 2019 14:26:31 +0100 Subject: [PATCH 5/6] chore: Update AUTHORS.rst --- AUTHORS.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS.rst b/AUTHORS.rst index 3c8dae41fef..d2642262656 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -60,6 +60,7 @@ The following wonderful people contributed directly or indirectly to this projec - `Oleg Sushchenko `_ - `Or Bin `_ - `overquota `_ +- `Paolo Lammens `_ - `Patrick Hofmann `_ - `Paul Larsen `_ - `Pieter Schutz `_ From 5a323d689a75eecc58b22a16e1f3a40e67bee651 Mon Sep 17 00:00:00 2001 From: Paolo Lammens Date: Fri, 1 Mar 2019 14:26:01 +0100 Subject: [PATCH 6/6] fix: Remove datetime.datetime.timestamp() call Python 2's datetime module doesn't have the datetime.timestamp() method. An equivalent calculation has been put in its place. --- telegram/ext/jobqueue.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/telegram/ext/jobqueue.py b/telegram/ext/jobqueue.py index fc930c73cbd..625c1f4bdec 100644 --- a/telegram/ext/jobqueue.py +++ b/telegram/ext/jobqueue.py @@ -508,7 +508,7 @@ def __lt__(self, other): return False -def _to_timestamp(time_, previous_t=None): +def _to_timestamp(t, previous_t=None): # """ # Converts a given time object (i.e., `datetime.datetime`, # `datetime.time`, `datetime.timedelta`, interval from now @@ -516,22 +516,28 @@ def _to_timestamp(time_, previous_t=None): # kwargs like `first` to a uniform format. # """ - previous_t = previous_t or time.time() + now = time.time() + previous_t = previous_t or now - if time_ is None: + if t is None: return None - elif hasattr(time_, 'timestamp'): - return time_.timestamp() - elif isinstance(time_, datetime.time): + + if isinstance(t, datetime.timedelta): + return previous_t + t.total_seconds() + + if isinstance(t, Number): + return previous_t + t + + if isinstance(t, datetime.time): date = datetime.date.today() - if time_ < datetime.datetime.today().time(): + if t < datetime.datetime.today().time(): date += datetime.timedelta(days=1) - return datetime.datetime.combine(date, time_).timestamp() - elif isinstance(time_, datetime.timedelta): - return previous_t + time_.total_seconds() - elif isinstance(time_, Number): - return previous_t + time_ - else: - raise TypeError('Unable to convert to timestamp') + t = datetime.datetime.combine(date, t) + + if isinstance(t, datetime.datetime): + # replacement for datetime.datetime.timestamp() for py2 + return (t - datetime.datetime.fromtimestamp(now)).total_seconds() + now + + raise TypeError('Unable to convert to timestamp')