fix alarm skill and make stateful #114
Conversation
| int_val += rom_val[s[i]] | ||
| return int_val | ||
|
|
||
| def get_advanced_matches(self, utt, have_time, when, when_utc, user_dow, alarm_list): |
There was a problem hiding this comment.
this is a really long method. could it be broken up?
There was a problem hiding this comment.
Not easily, however I notice it is about 60 lines long while the weather skill has a method called handle_week_weather() which is about 90 lines long; whats the diff?
There was a problem hiding this comment.
not in my refactor branch 😄
| @@ -87,6 +67,7 @@ def __init__(self): | |||
| self.flash_state = 0 | |||
| self.recurrence_dict = None | |||
| self.sound_name = None | |||
| self.skill_control.category = 'system' | |||
There was a problem hiding this comment.
I think using a "system" category for this skill could be confusing. This gives us two different definitions of a "system" skill. Could the category be something like "stateful" instead? How do we see categories being used in skill control?
There was a problem hiding this comment.
What is a system skill? How are they currently delineated?
Categories are used to maintain backward compatibility with existing skills and to allow skills defined as category=system to have priority over non system skills during converse.
|
OK, latest commit should address review input |
|
This is simply a string constant which describes the skill category. What was once a simple
self.category = 'system'
has now become
self.skill_control.category = 'system'
with a whole separate class (skill_control) to just hold 3 new attributes. It will next become
self.skill_control.category = 'who cares'
or could easily become
self.state_manager.something_besides_category_because_somebody_didnt_like_the_word_category_here = 'something_other_than_system_because_somebody_didnt_like_the_word_system'
I'll change it but every change like this runs the risk of breaking something and keeping these changes from getting pushed out and tested. In this particular case the new attribute value will undoubtedly not convey what the true intent of this value for this attribute is; namely this is a system skill to be treated differently than other non system level skills. This level of control does not exist in our current system as I know it.
This particular constant is used in nearly all the PRs I just submitted so I'll have to revisit every one and make the changes and then test to make sure nothing is broken and then update the PR. I suspect those changes will be ready sometime tomorrow so maybe we can get this code pushed out by the end of the week but I have already spent more time changing the style of this code and variable names then I did designing and implementing it.
…--------------------------------------------------------------------------------------------------------
Ken: I think the current "system" designation is used for things like
volume, wifi, and if i recall correctly, something like a half dozen
"invisible" Skills we never talk about. (Those need to be added to the list
of things to look at). [Obvs I might be remembering all this incorrectly. ]
Perhaps the category could be descriptive, rather than "categorical". E.g.
"interrupter" or "non-modal-notifier". But I doubt any scheme we come up
with off the cuff here will stand the test of time. My 2c: Just pick a name
that isn't "system" and we'll give the whole state system a thorough arch
review before Pass 2 on the essential Skills, with the benefit of knowing
better what we need to make things work, and what behaviors we want to define.
On May 18, 2021 1:25:22 PM ken-mycroft ***@***.***> wrote:
@ken-mycroft commented on this pull request.
In __init__.py:
> @@ -87,6 +67,7 @@ def __init__(self): self.flash_state = 0
> self.recurrence_dict = None self.sound_name = None
+ self.skill_control.category = 'system'
What is a system skill? How are they currently delineated?
Categories are used to maintain backward compatibility with existing skills
and to allow skills defined as category=system to have priority over non
system skills during converse.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I really hate this tool. I can not find previous comments for some reason. Anyway, there is a roman numeral routine in here and Chris commented he did not have to have that in his weather skill, however, Lingua Franca returns roman numerals for things like the fifth, the twenty sixth and if you don't handle them correctly you will get the behavior you have in the weather skill if you ask it what is the weather for the 21st or the seventh. |
clarification, lingua_franca does no such thing, the STT engine might return that, but LF contains no code to handle that at all, roman numerals are neither valid input nor output for LF |
# Conflicts: # __init__.py
# Conflicts: # __init__.py
Description
Fix alarm skill news skill interaction issues and make alarm skill stateful
If needed follow up with as much detail as required.
Type of PR
Testing
Set an alarm that goes off while news is playing. Stop should only stop the alarm, not the news
Documentation
Will need new VK tests