Refactor MycroftSkill #2256

forslund · 2019-08-13T13:56:28Z

Description

The intent of this PR is to reduce the size of the MycroftSkill class by separating some of the more complicated parts into separate classes and move some helper functions into separate files. The big things moved were event scheduling code, intent registration code, event handler tracking.

But there was some feature creep...

This PR intents to do the following

How to test

Contributor license agreement signed?

CLA [ Yes ]

pep8speaks · 2019-08-13T13:56:34Z

Hello @forslund! 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-08-30 11:32:31 UTC

chrisveilleux · 2019-08-28T18:16:55Z

Would you please put this in a Mycroft-core branch? I would like to be able to look at it in my IDE where it would be easier to navigate/compare.

forslund · 2019-08-28T18:42:24Z

Done, a copy of the refactor-split-mycroftskill branch is now available on the mycroftai/mycroft-core repo

chrisveilleux

OK, this is one hella big PR. I am not actually done with my review but I am starting to go cross-eyed looking at this code. There should be enough in here to keep you busy tomorrow ;) I will make some time to look at the rest tomorrow.

chrisveilleux · 2019-08-28T20:27:21Z

mycroft/skills/event_scheduler.py

@@ -250,3 +251,163 @@ def shutdown(self):
        self.clear_empty()
        # Store all pending scheduled events
        self.store()
+
+
+class EventSchedulerInterface:


Consider renaming to SkillEventScheduler. The current name seems too generic. At face value, this new class looks a lot like the existing EventScheduler class. At first I found myself asking why not combine the two. Maybe the class-level doc string needs more information to indicate why both classes are needed. Maybe the method names should have the word "skill" in them for clarity? schedule_skill_event?

Edit you have a point here, it's very entangled with the skills class right now. I'll do some rework to isolate it better.

mycroft/skills/event_scheduler.py

chrisveilleux · 2019-08-29T00:02:34Z

mycroft/skills/event_scheduler.py

+            self.scheduled_repeats.append(name)  # store "friendly name"
+
+        data = data or {}
+        self.skill.add_event(unique_name, handler, once=not repeat)


Why not track the skill's events in this class? This is the skill's event interface. Shouldn't all event-related logic for a skill be included in this class?

mycroft/skills/event_scheduler.py

mycroft/skills/mycroft_skill/mycroft_skill.py

chrisveilleux · 2019-08-29T01:41:40Z

mycroft/skills/mycroft_skill/mycroft_skill.py

-            'data': data
-        }
-        self.bus.emit(Message('mycroft.schedule.update_event', data=data))
+        self.event_scheduler.update_scheduled_event(name, data)


are you leaving this (and the next several methods like it) in here for backwards compatibility? if so, do we need to since this is a breaking release? if we do need to keep these, can we put a deprecation warning on them?

These are basically convenience methods right now. We'll need to have a chat after the release of the future of the MycroftSkill interface. There's been some discussion in the past but the current rule is the MycroftSkill class should contain all the methods a skill creator needs so it's my interpretation at the moment that these methods should remain as part of the interface.

chrisveilleux

Added a few comments and answered a few questions.

mycroft/skills/event_scheduler.py

mycroft/skills/intent_service_interface.py

mycroft/skills/mycroft_skill/__init__.py

mycroft/skills/mycroft_skill/event_container.py

mycroft/skills/mycroft_skill/mycroft_skill.py

test/unittests/skills/test_event_container.py

Group event scheduler interface logic into subclass

Split of decorators from the mycroft_skill file and create a submodule for mycroft_skill

This makes the register_intent decorator work for all types of intents, both Adapt and Padatious.

- Create separate method for regisetering mycroft system event handlers - Fix some string concatenations - Group acknowledge together with speak and speak_dialog

This also removes the _dir member which cointains the same information.

This container remembers registered skills and allows unregistering all handlers connected to a skill at shutdown.

- The data loading no longer require the bus - Add an intent service interface class for better testing - Update test cases

Move much of the complex methods for registering intents to the IntentServiceInterface to reduce bloat of MycroftSkill

Move file reading logic into skill_data.py

Use the more modern bus.wait_for_response()

- Remove once-logic from the default handler wrapper, no need to do it there. - Add docstring for handle_wrapper - add the on_start, on_end handlers and move skill logic to respective handlers.

chrisveilleux

As discussed, further clean up can occur over time. This is a great improvement to the MycroftSkill class. Well done.

mycroft/skills/event_scheduler.py

mycroft/skills/mycroft_skill/mycroft_skill.py

forslund added Status: Work in progress PR being actively worked on, not yet ready for review. Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality. labels Aug 13, 2019

devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 13, 2019

forslund force-pushed the refactor-split-mycroftskill branch 4 times, most recently from 98f3b6f to 57f9b10 Compare August 15, 2019 05:49

forslund force-pushed the refactor-split-mycroftskill branch 2 times, most recently from b1da14e to 4e5e20a Compare August 25, 2019 14:46

forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Aug 25, 2019

forslund requested review from chrisveilleux and davidwagnerkc August 25, 2019 20:36

forslund force-pushed the refactor-split-mycroftskill branch from 4e5e20a to 5ef1052 Compare August 27, 2019 09:16

chrisveilleux suggested changes Aug 29, 2019

View reviewed changes

forslund force-pushed the refactor-split-mycroftskill branch 6 times, most recently from 57c18fc to 9afcab9 Compare August 29, 2019 14:36

chrisveilleux suggested changes Aug 29, 2019

View reviewed changes

forslund force-pushed the refactor-split-mycroftskill branch 2 times, most recently from 4490526 to 3535e24 Compare August 29, 2019 19:21

forslund added 4 commits August 30, 2019 13:00

Populate root_dir in __init__ from file location

2e82f64

Create EventSchedulerInterface class

a3429db

Group event scheduler interface logic into subclass

Split of mycroft skill decorators into new file

0759d6f

Split of decorators from the mycroft_skill file and create a submodule for mycroft_skill

Move open_intent_envelope to intent_service

f5cdeb7

forslund added 16 commits August 30, 2019 13:00

Make register_intent decorator register all intent types

b28d63c

This makes the register_intent decorator work for all types of intents, both Adapt and Padatious.

Rename test file for mycroft_skill to match

2b3bd98

Minor cleanup of MycroftSkill

f655152

- Create separate method for regisetering mycroft system event handlers - Fix some string concatenations - Group acknowledge together with speak and speak_dialog

Set MycroftSkill.root_dir in __init__()

a187a17

This also removes the _dir member which cointains the same information.

Create container class for tracking skill handlers

7393d9a

This container remembers registered skills and allows unregistering all handlers connected to a skill at shutdown.

Refactor adapt intent data loading routines

245a4cb

- The data loading no longer require the bus - Add an intent service interface class for better testing - Update test cases

Use remove_all_listeners when cleaning

5402f8a

Move complexity to IntentServiceInterface

2d95ed9

Move much of the complex methods for registering intents to the IntentServiceInterface to reduce bloat of MycroftSkill

Reduce complexity of get_response()

e2ec2ef

Refactor translation methods

5e55864

Move file reading logic into skill_data.py

Further cleanup

f081404

Update docstrings and variable names of EventSchedulerInterface

d461ffe

Add comments and clean up IntentServiceInterface

c1d07bb

Update get_scheduled_event_status()

87d9512

Use the more modern bus.wait_for_response()

Remove skill logic from create_wrapper

cbf2fc6

- Remove once-logic from the default handler wrapper, no need to do it there. - Add docstring for handle_wrapper - add the on_start, on_end handlers and move skill logic to respective handlers.

Disentangle event scheduler interface from skill

077df67

forslund force-pushed the refactor-split-mycroftskill branch from 3535e24 to 077df67 Compare August 30, 2019 11:32

chrisveilleux approved these changes Aug 30, 2019

View reviewed changes

mycroft/skills/event_scheduler.py Show resolved Hide resolved

mycroft/skills/mycroft_skill/mycroft_skill.py Show resolved Hide resolved

forslund merged commit 75ad11b into MycroftAI:dev Aug 30, 2019

forslund deleted the refactor-split-mycroftskill branch November 28, 2019 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MycroftSkill #2256

Refactor MycroftSkill #2256

forslund commented Aug 13, 2019 •

edited

Loading

pep8speaks commented Aug 13, 2019 •

edited

Loading

chrisveilleux commented Aug 28, 2019

forslund commented Aug 28, 2019

chrisveilleux left a comment

chrisveilleux Aug 28, 2019

forslund Aug 29, 2019 •

edited

Loading

chrisveilleux Aug 29, 2019

forslund Aug 30, 2019

chrisveilleux Aug 29, 2019

forslund Aug 29, 2019

chrisveilleux left a comment

chrisveilleux left a comment

Refactor MycroftSkill #2256

Refactor MycroftSkill #2256

Conversation

forslund commented Aug 13, 2019 • edited Loading

Description

How to test

Contributor license agreement signed?

pep8speaks commented Aug 13, 2019 • edited Loading

Comment last updated at 2019-08-30 11:32:31 UTC

chrisveilleux commented Aug 28, 2019

forslund commented Aug 28, 2019

chrisveilleux left a comment

Choose a reason for hiding this comment

chrisveilleux Aug 28, 2019

Choose a reason for hiding this comment

forslund Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

chrisveilleux Aug 29, 2019

Choose a reason for hiding this comment

forslund Aug 30, 2019

Choose a reason for hiding this comment

chrisveilleux Aug 29, 2019

Choose a reason for hiding this comment

forslund Aug 29, 2019

Choose a reason for hiding this comment

chrisveilleux left a comment

Choose a reason for hiding this comment

chrisveilleux left a comment

Choose a reason for hiding this comment

forslund commented Aug 13, 2019 •

edited

Loading

pep8speaks commented Aug 13, 2019 •

edited

Loading

forslund Aug 29, 2019 •

edited

Loading