Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Conversation

@LearnedVector
Copy link
Contributor

@LearnedVector LearnedVector commented Jul 28, 2017

==== 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.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 41.225% when pulling 0c0bc6f on feature/skill-configuration into edbd8ed on dev.

@LearnedVector LearnedVector mentioned this pull request Jul 28, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 41.173% when pulling 05852f3 on feature/skill-configuration into edbd8ed on dev.

Copy link
Collaborator

@forslund forslund left a 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
Copy link
Collaborator

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?

Copy link
Contributor Author

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):
Copy link
Collaborator

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...

Copy link
Contributor Author

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)
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

@forslund forslund added Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. Epic labels Aug 1, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 41.314% when pulling 48b26d1 on feature/skill-configuration into edbd8ed on dev.

@forslund forslund merged commit e7c55aa into dev Aug 2, 2017
@aatchison aatchison deleted the feature/skill-configuration branch August 2, 2017 18:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants