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 manager #2237

merged 103 commits into from Aug 20, 2019


Copy link

commented Jul 26, 2019


Refactoring of the module.

  • Moved all the MSM logic to install/update skills into a new SkillUpdater class.
  • Moved all the load/reload logic into a new SkillLoader class
  • Added a bunch of unit tests to manager, updater and loader.

How to test

This is a refactor so all functionality should work as it did before these changes.

Contributor license agreement signed?

CLA [yes]

chrisveilleux added 30 commits Jul 15, 2019
- added some docstrings
- put config retrieval in a function
- added log messages
- fixed spelling error
- renamed variable that shadowed built-in
- reordered imports
- refactored docstrings for consistency
- renamed camel case variables to use snake case
- renamed the message bus client
- abstracted message bus config loading so service and client can use same code.
… it is a backport that inherits from the new MessageBusClient class. Added a deprecation comment as well.
… may cause breakage. added todo to include in next major release.
- broke big functions up into classes/methods
- reworked the multiple calls to is_paired() into a single call to minimize API calls and improve performance a bit
- added more documentation
- added more logging
- other miscellaneous cleaning stuff
…into this class... for now.
…to refactor-skill-service
@chrisveilleux chrisveilleux requested a review from forslund Aug 15, 2019

This comment has been minimized.

Copy link

commented Aug 19, 2019

There are some issues handling reloading of skills along with a couple of issues with the activate/deactivate handlers. I've looked it over and posted a suggestion, see commit list here. If you're okay with the changes just cherry pick those commits to your branch.

There is some possibilities of threading issues when calling the activate/deactivate methods but those will basically only hit skill developers so we can postpone that.

I've also skipped code style check we can refactor this further at a later date.

forslund added 4 commits Aug 19, 2019
- Separate reload required check from performing reload to make logic easier
to follow
- Track skills that failed to load to handle infinite loop at first load
if skill fails to load
- Allow reloading skills that has failed to load

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@forslund I merged your fixes into this branch. Thanks for putting it through its paces.

@forslund forslund force-pushed the refactor-skill-manager branch 2 times, most recently from 57a3956 to eeabbe5 Aug 20, 2019
- create activate, deactivate and unload methods for skill_loader
- add sanity checks before activating and deactivating skills
- Update activation/deactivation test cases
@forslund forslund force-pushed the refactor-skill-manager branch from eeabbe5 to 711cc6c Aug 20, 2019
- Fix failures introduced when creating unload method on SkillLoader
- Fix failing test cases where returnvalue is needed
- Fix mock config to allow updates
- Fix invalid assert in
- Replace assert_called_once with assert_called_once_with to make python
3.5 tests work

Fix rest of python 3.5 tests
@forslund forslund force-pushed the refactor-skill-manager branch from 711cc6c to 2ae5ef4 Aug 20, 2019

This comment has been minimized.

Copy link

commented Aug 20, 2019

Finally figured out why your test cases were failing on Python 3.5. Merging this now.


This comment has been minimized.

Copy link

commented Aug 20, 2019

The commit history here is so messed up I'll do a squash to make a sane commit :/

@forslund forslund merged commit 3bd3dd1 into dev Aug 20, 2019
3 of 4 checks passed
3 of 4 checks passed
License Compliance 11 issues found
:-) Build finished.
continuous-integration/travis-ci/pr The Travis CI build passed
coverage/coveralls Coverage increased (+1.3%) to 53.014%
@chrisveilleux chrisveilleux deleted the refactor-skill-manager branch Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
4 participants
You can’t perform that action at this time.