Feature/status event scheduler #1193
Conversation
==== Tech Notes ==== This allows you to remove message bus messages inside core.py. When canceling scheduling events, the message bus messages were not removed which could caused duplicate listeners and handlers for the same intent. Adding remove_event function removes the actual messages from the bus to prevent potential duplicates.
mycroft/skills/core.py
Outdated
|
|
||
| # this is needed because message.data could return None | ||
| # and that would make the while loop infinite | ||
| self.event_status = "None" |
There was a problem hiding this comment.
I would personally have used the None type as a default value
mycroft/skills/core.py
Outdated
| self.emitter.once(emitter_name, callback) | ||
| self.emitter.emit(Message('mycroft.scheduler.get_event', data=data)) | ||
|
|
||
| while True: |
There was a problem hiding this comment.
This should probably have a timeout of say two seconds, and do sleeps between checks to avoid hogging the cpu:
start_wait = time.time()
while event_status is None and time.time() - start_wait < 2.0:
time.sleep(0.1)
return event_status(Having it return None on failure is a common approach in python)
There was a problem hiding this comment.
Maybe it would be even better if it raised an exception on timeout. Not sure which one would be approprate. IOError or some from the websocket client?
Not critical, you decide which way you like best.
|
@forslund did some changes. Also i realized this PR also contains the remove_event code from the other PR. Should we just close the other PR and have this one for both? |
|
Yes, might be better to close that. |
forslund
added
the
CLA: Yes
mycroft/skills/core.py
Outdated
| event_status = [None] | ||
|
|
||
| def callback(message, event_status): | ||
| event_time = int(message.data[0][0]) |
There was a problem hiding this comment.
Needs check if message.data is not None since that is a valid response if event doesn't exist. If it is None set event_status[0] to False or 0 to indicate that it doesn't exist.
(Timeout exception is a bit misleading if the actual issue is that the handler doesn't exist)
mycroft/skills/core.py
Outdated
| # making event_status an object so it's refrence can be changed | ||
| event_status = [None] | ||
|
|
||
| def callback(message, event_status): |
There was a problem hiding this comment.
event_status don't seem to be needed as an argument
There was a problem hiding this comment.
this is actually needed to be able to change the value of the event_status. If this is not passed this way to the callback then when we say event_status = whatever, it won't reference the same variable.
There was a problem hiding this comment.
Hmm, when I tested it wasn't needed. As long the outer_var is a mutable (in this case a list) it should work.
def outer():
outer_var = [None]
def inner():
outer_var[0] = 'Modified'
inner()
print outer_var[0]
outer()I'll retest but it's not really important, this way works as well.
There was a problem hiding this comment.
Ok retried and it doesn't seem necessary but never mind both ways work.
There was a problem hiding this comment.
ahh ok thanks!
|
@forslund made some changes and also changed the function name for scheduled events to be more expressive of what it's doing. let me know what you thiink |
mycroft/skills/core.py
Outdated
| while event_status[0] is None and time.time() - start_wait < 3.0: | ||
| time.sleep(0.1) | ||
| if event_status[0] is False: | ||
| raise Exception("Scheduled Event not found for {}".format(name)) |
There was a problem hiding this comment.
Sorry if I was unclear (or confused, or both) earlier. I meant that the function should return None if there is no scheduled event.
A use case I can imagine is to check if an event is scheduled at all and checking if The self.get_scheduled_event_status(event) is None is a pretty simple way to do it.
Timeout is an actual error while "event isn't scheduled" is a valid status (at least in my mind).
What's your opinion?
There was a problem hiding this comment.
makes a lot of sense to me!
|
@forslund let's try this again! |
|
Awesome, looks and works great! Merging |
This PR gives an api for skill developers to query event status from the event scheduler