-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SchedulerBase polish, typings, test #361
Conversation
return default_now() | ||
|
||
@abstractmethod | ||
def schedule(self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious, is it worth to re-declare schedule
, schedule_relative
& schedule_absolute
as abstract methods while these are already declared in typing.Scheduler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe not strictly necessary... Although it might help to keep the generated docs and help()
complete.
I guess I included this mainly because of an idea I have that I’m playing around with, which would simplify all of the classes that extend SchedulerBase
by putting some often-repeated logic (schedule methods being defined in terms of one another) in the base class.
Having these here at this point would make that second step a lot easier / readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool 👍
Ugh I just noticed I accidentally included a change in virtualtime scheduler here... It should have been in that other PR. Sorry about that, I’ve been juggling branches and commits and this fell through. Please ignore that part, I’ll transplant it later. |
c2c11a6
to
0fedf87
Compare
0fedf87
to
2910e90
Compare
FYI, I removed the bit which was not supposed to be in this PR, and I added a sentence about |
2910e90
to
6b1a628
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Thanks for review! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
I am once again trying to separate moderately sized bits off a branch with way too many changes in it...