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

Move #core Skills out of DEFAULT and into each DEFAULT.platform file,… #452

Merged
merged 7 commits into from
Jul 19, 2018

Conversation

KathyReid
Copy link
Contributor

This PR is based on discussions w/ @el-tocino, @AIIX and @penrods around the need to slightly modify the DEFAULT file so that it doesn't overwrite Skills that are customised on a per-platform basis, like skill-wiki is for the KDE platform.

This is a really simple change and should be non-breaking, but definitely needs an experienced eye to review because of the large scope that the DEFAULT.* files have.

Suggested test sequence to validate this PR:

  • Using the KDE enclosure, remove all Skills. Run msm default to bring in default Skills.

  • Ensure that the offending Skills (stocks, weather - sorry I couldn't find the wikipedia Skill in the #core list) are not installed.
    This tests the intended outcome for KDE

  • Using the Mark 1 enclosure, remove all Skills. Run msm default to bring in default Skills.

  • Ensure that all the default Skills are loaded.
    This tests that we don't adversely impact other Skills with the change

@KathyReid KathyReid added approved Ready to be merged needs validation Needs validatiion by another Skills Team Member or Community Member trying it under review Under review by the Skills Team or Community and removed approved Ready to be merged labels Jul 9, 2018
@KathyReid
Copy link
Contributor Author

One thing that I think might be needed here is a DEFAULT.linux file for linux- or git-based installs. I don't know how to implement that though. Very happy to take pointers.

@penrods
Copy link
Contributor

penrods commented Jul 13, 2018

Are there a reasons you decided not to include the first three defaults in the various specific platforms? Is the verbal volume control not useful even on those platforms, for example?

@KathyReid
Copy link
Contributor Author

@penrods good pickup, that wasn't a conscious decision, that was poor attention to detail.
I will add those 3 x Skills to each platform; it makes sense to add them.

add
mycroft-playback-control
 mycroft-speak
 mycroft-volume
add 
mycroft-playback-control
 mycroft-speak
 mycroft-volume
add
mycroft-playback-control
mycroft-speak
mycroft-volume
Add common Skills back into DEFAULT-SKILLS as per Skill Team Mtg 19th July
@penrods
Copy link
Contributor

penrods commented Jul 19, 2018

LGTM!

@penrods penrods merged commit dcd55da into 18.02 Jul 19, 2018
@penrods penrods deleted the issue/412 branch July 19, 2018 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs validation Needs validatiion by another Skills Team Member or Community Member trying it under review Under review by the Skills Team or Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants