Adds OneShotTimer and OneShotDispatch Timer#10
Adds OneShotTimer and OneShotDispatch Timer#10JeanAzzopardi wants to merge 2 commits intocomyar:mainfrom
Conversation
Refactors ExecutionClosures to RepeatedExecutionClosure and OneShotExecutionClosure
bdaf3be to
084ed4d
Compare
|
Very cool, thanks for the contribution. I wonder how much utility this would provide over Anyway, I'll be traveling through next week so I won't be able to seriously review this for a while, but overall I like the idea of including this. |
|
Thanks for your comment, I'm working on an app with multiple timers, some repeating, some one-shot (scheduled events as you said). I liked the API of Chronos, so I thought it would be great to keep the same API for one-shot timers. |
|
|
||
| - parameter now: true, if the timer should fire immediately. | ||
| */ | ||
| public func start(now: Bool) { |
There was a problem hiding this comment.
Extending the Timer protocol becomes sort of awkward because of the start function here, right? For a OneShotTimer, the now argument doesn't really make much sense.
I think what we should do is change the declaration of the start function in Timer from func start(now: Bool) to func start(). Then we add func start(now: Bool) to the RepeatingTimer protocol. What do you think?
|
Sorry for the late response to this, overall it looks good. Only some minor comments throughout, really like that you also added tests and maintained the code style 😄. I'd like to see the changes to the Regarding naming, I'm indifferent either way we go on it, I was just interested in getting your thoughts on it. Thanks again for the contribution! |
It might be a good idea to have a OneShotTimer, i.e. a timer that can only be run once after a certain delay.
I've done some refactoring to RepeatingTimer in order to change the type of the closure. , so now there are 2 closure types:
I realise that this is a non-trivial change, but I'd welcome any feedback.