feature - enabling a time to execution property on Routines

PythonistaGuild / TwitchIO

An Async Bot/API wrapper for Twitch made in Python.

https://twitchio.dev

MIT License

802 stars 163 forks source link

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

Open rsivilli opened 1 year ago

rsivilli commented 1 year ago

Pull request summary

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

Checklist

[x] If code changes were made then they have been tested.
- [x] I have updated the documentation to reflect the changes.
- [x] I have updated the changelog with a quick recap of my changes.
[ ] This PR fixes an issue.
[x] 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, ...)

chillymosh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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