Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix converse and snooze #110
base: 20.08
Are you sure you want to change the base?
Fix converse and snooze #110
Changes from 5 commits
da6213e
2e4cdf6
38abc25
4114550
a0e1ee8
498bd86
67b71d2
5256864
5994995
3fa1bde
e43221b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is the intention that this will fully replace
_get_alarm_matches()
or is it intended to be distinct in some way?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.
It is used when the normal flow can not find any
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've logged this as an issue on that repo. Seems safe enough to do if a valid date exists, though plenty hacky and we should remove it asap.
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.
That parser's being rewritten, because there are all sorts of problems like this. I'd suggest adding a TODO for MycroftAI/lingua-franca#96 which should make this unnecessary, and provide more robust coverage for the same conditions.
Obviously wouldn't be prudent to offer a timetable on that, as there are multiple design choices up in the air in chat, and several higher priorities for everyone on LF. Still, Jarbas has written a lot of the code. He might be due to push, as well.
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.
yes... :)
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.
be better to use mycroft.util.time.to_utc()
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.
could this be more descriptive, eg
utterance_has_time
?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.
This means we believe we have a time. Its not strictly utterance dependent. It is derived. You may rename it if you like.
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.
this would trigger for any utterance does not contain Stop or Snooze. I don't think we want to do anything here at the moment.
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.
Unless we want to prompt the user - "do you want to turn off the alarm too?"
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 think we should only consume the phrase if we match on StopBeeping or Snooze - otherwise we are consuming every utterance if an alarm is expired.
This means Mycroft won't respond to anything until you either turn off the alarm or snooze.
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.
This is an interesting point. We are technically in a converse state within an alarm activation. Do we want to open ourselves up to allowing other things to happen before we resolve the current issue with the active alarm? I don't think I know the answer to this so I simply get the code to pass the VK tests which are basically the spec. Now if you would like I suspect you could create a new VK test which initially fails and demonstrates the behavior you would like to alter. I would be open to this.
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.
If we're taking this out let's just delete it rather than commenting it out.
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.
done