-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Feature/skill configuration #933
Conversation
…feature/skill-configuration
…feature/skill-configuration
…feature/skill-configuration
forslund
left a comment
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.
Mostly looks good! I'd like to keep the settings_file parameter to make tests easier and to allow skill creators to create their own files if they want.
|
|
||
| @property | ||
| def _is_stored(self): | ||
| return hash(str(self)) == self.loaded_hash |
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 there a reason for removing this check?
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.
resolved. I put it back and used it in poll_settings
| settings_file (str): Path to storage file | ||
| """ | ||
| def __init__(self, settings_file): | ||
| def __init__(self, directory): |
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'd like to keep the settings_file option to keep this more a general class that can be instantiated multiple times with multiple settings-files. Can you append a .meta to the settings_file path to keep the api?
Makes testing easier 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.
resolved
| def test_load_existing(self): | ||
| s = SkillSettings(join(dirname(__file__), 'settings', 'existing.json')) | ||
| self.assertEqual(len(s), 4) | ||
| self.assertEqual(s['test_val'], 1) |
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 a kind of important test case and should be implemented 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.
resolved
==== Fixed Issues ====
NONE
==== Tech Notes ====
settings.py is extended to make REST api calls to home.mycroft.ai to allow skill configurations. currently does a 60 second poll to home.mycroft.ai to pull changes skill configuration changes.
==== Documentation Notes ====
Mycroft skill developers can now add a settingsmeta.json file into their skills, and the core will automatically upload the file to home.mycroft.ai for you to input the settings. This will then go back to the core and create a settings.json file that can be used for different settings in the skills.
settingsmeta.json examples:
{ "identifier": "PandoraSkill", "name": "Pandora", "skillMetadata": { "sections": [ { "name": "Login", "fields": [ { "name": "email", "type": "email", "label": "Email", "value": "" }, { "name": "password", "type": "password", "label": "Password", "value": "" } ] } ] } }==== Localization Notes ====
NONE
==== Environment Notes ====
NONE
==== Protocol Notes ====
settings.json, settingsmeta.json are the default file names the core will be looking at inside your skills folder.