feature - enabling a time to execution property on Routines #416

rsivilli · 2023-08-14T12:03:11Z

Pull request summary

Proposed attempt to exposing a "time to next execution" property within Routines

Checklist

If code changes were made then they have been tested.
- I have updated the documentation to reflect the changes.
- I have updated the changelog with a quick recap of my changes.
This PR fixes an issue.
This PR adds something new (e.g. new method or parameters).
This PR is a breaking change (e.g. methods or parameters removed/renamed)
This PR is not a code change (e.g. documentation, README, ...)

AbstractUmbra

Whilst I respect this is still in draft, it has some issues already:

time_till_execution is not proper English, perhaps you'd mean to use next_execution_time?

There are more in the review steps below.

twitchio/ext/routines/__init__.py

AbstractUmbra · 2023-08-14T12:09:33Z

twitchio/ext/routines/__init__.py

-
+
+    @property
+    def time_till_execution(self) -> datetime.timedelta:


See review comment on this one.

Actually, I'm going back through this and it sort of clicked. The naming of the property could be confusing.

This does expose a next_event_time. As an aside, that should probably be marked as private.

How would you feel about something like time_until_next_event? While the docstring and typing should be pretty clear, I figure the naming could also be improved to emphasize that this is a duration estimate.

twitchio/ext/routines/__init__.py

chillymosh · 2023-08-14T12:17:58Z

Have you tested all variations of multiple routines running simultaneously with some being stopped, the execution time being changed mid run, cancelled etc.

We also run this with black -l 120

rsivilli · 2023-08-14T13:11:39Z

Have you tested all variations of multiple routines running simultaneously with some being stopped, the execution time being changed mid run, cancelled etc.

We also run this with black -l 120

Still working through test cases. Happy path for what I need it for seems to be stable with start/stops but it's by no means exhaustive yet. Are there tests in the repo or elsewhere to improve that coverage or at least ensure I'm doing no harm?

Copy on the use of black.

rsivilli · 2023-08-14T13:18:26Z

Whilst I respect this is still in draft, it has some issues already:

time_till_execution is not proper English, perhaps you'd mean to use next_execution_time?

There are more in the review steps below.

I think the name came from the discord discussion. Will update in next push

AbstractUmbra

One missed change

docs/changelog.rst

AbstractUmbra · 2023-08-14T20:26:42Z

The checks are failing due to using 3.10+ Union syntax (T | None) when the minimum python version is 3.7. You may need to switch it out for typing.Union[T, None].

rsivilli · 2023-08-19T21:30:33Z

Have you tested all variations of multiple routines running simultaneously with some being stopped, the execution time being changed mid run, cancelled etc.

We also run this with black -l 120

./test.py is my sort of body of evidence that, yes, I believe that this has now been tested against most use cases (in the hackiest way). It will be removed when this leaves draft, but I wasn't sure how else to prove that.

AbstractUmbra

Some queries around style choices and whatnot, that's all!

AbstractUmbra · 2023-09-08T07:02:23Z

docs/changelog.rst

@@ -1,11 +1,12 @@
-:orphan:
+orphan:


Directive should be :orphan:, no?

AbstractUmbra · 2023-09-08T07:02:28Z

docs/changelog.rst


 Master
 ======
 - TwitchIO
    - Bug fixes
        - Fix IndexError when getting prefix when empty message is sent in a reply.
-
+    - Additioons


Suggested change

- Additioons

- Additions

AbstractUmbra · 2023-09-08T07:02:57Z

docs/changelog.rst

@@ -24,6 +25,7 @@ Master
        - Added :func:`~twitchio.Client.fetch_content_classification_labels` along with :class:`~twitchio.ContentClassificationLabel`
        - Added :attr:`~twitchio.ChannelInfo.content_classification_labels` and :attr:`~twitchio.ChannelInfo.is_branded_content` to :class:`~twitchio.ChannelInfo`
        - Added new parameters to :func:`~twitchio.PartialUser.modify_stream` for ``is_branded_content`` and ``content_classification_labels``
+


Why was this newline added? Did it cause an issue in the building of the documentation?

AbstractUmbra · 2023-09-08T07:03:43Z

twitchio/ext/routines/__init__.py

@@ -211,7 +215,6 @@ def restart_when_over(fut, *, args=args, kwargs=kwargs):

        if self._can_be_cancelled():
            self._task.add_done_callback(restart_when_over)
-


Why was this newline removed?

AbstractUmbra · 2023-09-08T07:05:03Z

There's also a merge conflict that must be resolved be we can merge.

AbstractUmbra requested changes Aug 14, 2023

View reviewed changes

chillymosh reviewed Aug 14, 2023

View reviewed changes

twitchio/ext/routines/__init__.py Outdated Show resolved Hide resolved

chillymosh reviewed Aug 14, 2023

View reviewed changes

twitchio/ext/routines/__init__.py Outdated Show resolved Hide resolved

AbstractUmbra requested changes Aug 14, 2023

View reviewed changes

docs/changelog.rst Outdated Show resolved Hide resolved

rsivilli force-pushed the rms/freature-time-remaining-routines branch from f396e88 to d918338 Compare August 19, 2023 13:41

rsivilli added 8 commits August 19, 2023 16:14

initial cut

6328f17

updating changelog

5032128

updating comments to reflect train of thought

9f4ccd1

updates based on feedback in draft PR

5dd5564

syncing with new name

43dd728

handling cases where time for routine doesn't include tzinfo

6e7ca4d

using Optional for typing for 3.7 compatibility

403fb54

typo

3b017b0

rsivilli force-pushed the rms/freature-time-remaining-routines branch from a2a1ac1 to 3b017b0 Compare August 19, 2023 20:14

rsivilli added 5 commits August 19, 2023 16:19

deleting

a32cae7

changes based on tests

227533f

changing property name to better reflect what it is

0bfb988

updating changelog

a676600

accounting for stopping and ending iterations

02cc9c0

rsivilli added 2 commits August 20, 2023 06:50

missed a |

899dafc

removing test.py in prep for final review

766b9db

rsivilli marked this pull request as ready for review August 21, 2023 14:41

rsivilli requested a review from AbstractUmbra August 22, 2023 00:21

AbstractUmbra requested changes Sep 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature - enabling a time to execution property on Routines #416

feature - enabling a time to execution property on Routines #416

rsivilli commented Aug 14, 2023 •

edited by AbstractUmbra

AbstractUmbra left a comment

AbstractUmbra Aug 14, 2023

rsivilli Aug 14, 2023

chillymosh commented Aug 14, 2023 •

edited

rsivilli commented Aug 14, 2023

rsivilli commented Aug 14, 2023

AbstractUmbra left a comment

AbstractUmbra commented Aug 14, 2023

rsivilli commented Aug 19, 2023

AbstractUmbra left a comment

AbstractUmbra Sep 8, 2023

AbstractUmbra Sep 8, 2023

AbstractUmbra Sep 8, 2023

AbstractUmbra Sep 8, 2023

AbstractUmbra commented Sep 8, 2023



		@property
		def time_till_execution(self) -> datetime.timedelta:

		@@ -211,7 +215,6 @@ def restart_when_over(fut, *, args=args, kwargs=kwargs):

		if self._can_be_cancelled():
		self._task.add_done_callback(restart_when_over)

		@@ -1,11 +1,12 @@
		:orphan:
		orphan:

feature - enabling a time to execution property on Routines #416

Are you sure you want to change the base?

feature - enabling a time to execution property on Routines #416

Conversation

rsivilli commented Aug 14, 2023 • edited by AbstractUmbra

Pull request summary

Checklist

AbstractUmbra left a comment

Choose a reason for hiding this comment

AbstractUmbra Aug 14, 2023

Choose a reason for hiding this comment

rsivilli Aug 14, 2023

Choose a reason for hiding this comment

chillymosh commented Aug 14, 2023 • edited

rsivilli commented Aug 14, 2023

rsivilli commented Aug 14, 2023

AbstractUmbra left a comment

Choose a reason for hiding this comment

AbstractUmbra commented Aug 14, 2023

rsivilli commented Aug 19, 2023

AbstractUmbra left a comment

Choose a reason for hiding this comment

AbstractUmbra Sep 8, 2023

Choose a reason for hiding this comment

AbstractUmbra Sep 8, 2023

Choose a reason for hiding this comment

AbstractUmbra Sep 8, 2023

Choose a reason for hiding this comment

AbstractUmbra Sep 8, 2023

Choose a reason for hiding this comment

AbstractUmbra commented Sep 8, 2023

rsivilli commented Aug 14, 2023 •

edited by AbstractUmbra

chillymosh commented Aug 14, 2023 •

edited