search

johanpalmqvist / skill-squeezebox

Apache License 2.0
2 stars 4 forks source link

improve fetching of settings #3

Closed krisgesling closed 4 years ago

krisgesling commented 4 years ago

Hi Johan,

I saw some people having issues with getting the settings block to show up at Home.mycroft.ai so wanted to suggest a slight modification to how this is handled when the Skill is initialized.

self.settings_change_callback defines a function to be called anytime the web settings are updated. So this PR separates all the settings fetching along with the call to get_sources to that callback. I also added settings.json to .gitignore so that no one accidentally commits their personal settings.

I don't have a Squeezebox server setup so haven't been able to test this end to end. It does however fix the settings fetching.

Thanks Kris

johanpalmqvist commented 4 years ago

Hi Kris,

Thanks! Since I currently deploy the settings with Ansible outside of Home.mycroft.ai I haven't run into this issue and wasn't aware of it.

Unfortunately I can't test your changes end to end right now but they look good to me so I'll merge and hope for a better user experience and test it as soon as possible.

Thanks again! Johan

krisgesling commented 4 years ago

Yeah nice, I hadn't heard of anyone using Ansible for settings.

Based on the Forums, it looks like a number of people are already happily using this Skill. If you think it is ready for broader adoption then please submit it for inclusion in the Mycroft Marketplace by running: mycroft-msk submit /opt/mycroft/skills/your-skills-directory

This will automatically generate a PR and the Skill Testing Team will review it in accordance with our Skills Acceptance Process