Skip to content
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

Refactor/skill settings split #2287

Merged
merged 13 commits into from Sep 17, 2019

Conversation

@chrisveilleux
Copy link
Member

commented Sep 3, 2019

Description

Complete rethink of how we deal with settings and settingsmeta in core

How to test

Make changes to settingsmeta.json for a skill. Confirm that the changes are accurately reflected on the backend. Make changes to skill settings at account.mycroft.ai. Confirm that changes are accurately reflected in settings.json

Contributor license agreement signed?

CLA [yes]

@forslund

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

What's breaking here, The callback registration and the .store() methods? Is there a way of preserving these so we don't break the API without depreciation warning?

The save_settings should only occur if the settings were changed.

Let's move as much of the logic for this outside of the mycroft_skill.

Also let's drop the refactoring commits so it's easier to review.

@chrisveilleux

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

Implemented the change you suggested to make the skill API right again.

I don't think I can move any more of the logic outside of MycroftSkill. The only thing I really added was a way to deal with settings changes when they occur.

Added logic to only call save_settings if settings change

@forslund forslund force-pushed the refactor/skill-settings-split branch from 1956de2 to 84e4fea Sep 5, 2019
Copy link
Member

left a comment

Some things I've noted when running the branch prematurely. You're probably aware of them but in case you weren't I'll flag them to save some time.

Also setting the line skill_loader.py L224 should be removed. I think we'll just drop this "feature".

mycroft/skills/settings.py Show resolved Hide resolved
mycroft/skills/settings.py Show resolved Hide resolved
mycroft/api/__init__.py Outdated Show resolved Hide resolved
@forslund forslund force-pushed the refactor/skill-settings-split branch 6 times, most recently from 81a5cac to cfb9594 Sep 10, 2019
@forslund forslund force-pushed the refactor/skill-settings-split branch from cfb9594 to 40710de Sep 10, 2019
@forslund forslund self-requested a review Sep 10, 2019
Copy link
Member

left a comment

Seems to be working fine

@forslund forslund force-pushed the refactor/skill-settings-split branch from 40710de to f7c4700 Sep 17, 2019
@forslund

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Backend has been updated. Merging this now.

@forslund forslund merged commit a41284e into dev Sep 17, 2019
3 checks passed
3 checks passed
:-) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 53.793%
Details
@forslund forslund deleted the refactor/skill-settings-split branch Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.