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 service #2220

Merged
merged 41 commits into from Jul 26, 2019

Conversation

@chrisveilleux
Copy link
Member

commented Jul 18, 2019

Description

Refactor the code that starts the skill service and primes the device when connected to the internet. Mostly just reorganized the code but also made a change to the logic that determines if a device is paired. The change reduces the amount of API calls to one.

How to test

This is a refactor so the unit tests need to pass and the skill service needs to work the same as it has.

Contributor license agreement signed?

CLA [yes]

chrisveilleux added 28 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 review from forslund and davidwagnerkc Jul 18, 2019
@pep8speaks

This comment has been minimized.

Copy link

commented Jul 18, 2019

Hello @chrisveilleux! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-26 06:24:44 UTC
@forslund

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

There are some really good things here (and a few things I don't quite agree with). Will you let me take a stab at making an alternative approach during the weekend based on this?

@chrisveilleux

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

There are some really good things here (and a few things I don't quite agree with). Will you let me take a stab at making an alternative approach during the weekend based on this?

Have at it. Interested to see the differences between our approaches.

Copy link
Member

left a comment

Ok this is much more to my liking. A couple of minor things (see separate comments).

I wonder if we need to have the _ at the start of functions since this is a __main__.py and it's quite clear it should not be imported?

mycroft/skills/__main__.py Show resolved Hide resolved
mycroft/skills/__main__.py Outdated Show resolved Hide resolved
mycroft/skills/__main__.py Outdated Show resolved Hide resolved
mycroft/skills/__main__.py Show resolved Hide resolved
mycroft/skills/__main__.py Outdated Show resolved Hide resolved
mycroft/skills/__main__.py Outdated Show resolved Hide resolved
@@ -230,15 +230,14 @@ def load_config_stack(configs=None, cache=False):
return base

@staticmethod
def init(ws):

This comment has been minimized.

Copy link
@forslund

forslund Jul 24, 2019

Member

I like the rename but we might want to keep the init method as well for backwards compatibility

This comment has been minimized.

Copy link
@chrisveilleux

chrisveilleux Jul 24, 2019

Author Member

I changed all the core uses of this. Are there other repos that could call this?

This comment has been minimized.

Copy link
@forslund

forslund Jul 24, 2019

Member

Technically things like third party enclosures may use it or other things build for working with mycroft-core.

I do think we can have a rather short depreciation period though and remove it outright in 19.08 since it's a rarely touched thing.

This comment has been minimized.

Copy link
@chrisveilleux

chrisveilleux Jul 24, 2019

Author Member

OK. will add an init method back that calls the new method and add a deprecation warning.

This comment has been minimized.

Copy link
@chrisveilleux

chrisveilleux Jul 24, 2019

Author Member

ugh! since it is a static method, I had to actually duplicate the method rather than have one call the other. I added a deprecation message. How do we track things like this that will be breaking changes in 19.08? Do we just search the code base for comments that mention 19.08?

This comment has been minimized.

Copy link
@forslund

forslund Jul 26, 2019

Member

That's basically it yeah TODO: 19.08 or similar comments

@forslund

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Also can you do a rebase against dev? The commit history looks really weird...

@chrisveilleux

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Also can you do a rebase against dev? The commit history looks really weird...

done

@forslund

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

I think this looks pretty good, gonna do some final testruns. I want the current release to be out for 24 hours and then I'll merge this on Friday if no hot-fixes are needed.

@forslund forslund merged commit 0bde1bc into dev Jul 26, 2019
3 checks passed
3 checks passed
:-) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 50.131%
Details
@forslund forslund deleted the refactor-skill-service branch Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.