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.

Sign up for GitHub

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

Jump to bottom

refactor!:drop mycroft #235

Merged
merged 3 commits into from
Oct 15, 2024
Merged

refactor!:drop mycroft #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24368b1
refactor!:drop mycroft
JarbasAl Sep 18, 2024
66e5d8a
tests
JarbasAl Oct 15, 2024
edcc0ea
port extra tests from ovos-core
JarbasAl Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions ovos_workshop/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +0,0 @@
from ovos_workshop.app import OVOSAbstractApplication
from ovos_workshop.decorators import *
from ovos_workshop.decorators.killable import killable_event, \
AbortEvent, AbortQuestion
from ovos_workshop.decorators.layers import IntentLayers
44 changes: 0 additions & 44 deletions ovos_workshop/decorators/compat.py
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion ovos_workshop/decorators/ocp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from ovos_workshop.backwards_compat import MediaType, PlayerState, MediaState, MatchConfidence, \
from ovos_utils.ocp import MediaType, PlayerState, MediaState, MatchConfidence, \
PlaybackType, PlaybackMode, LoopState, TrackState
JarbasAl marked this conversation as resolved.
Show resolved Hide resolved


Expand Down
3 changes: 1 addition & 2 deletions ovos_workshop/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@

class SkillSettingsManager:
def __init__(self, skill):
from ovos_workshop.skills.base import BaseSkill
self.download_timer: Optional[Timer] = None
self.skill: BaseSkill = skill
self.skill = skill
self.api = DeviceApi()
self.remote_settings = \
RemoteSkillSettings(self.skill_id,
Expand Down
14 changes: 5 additions & 9 deletions ovos_workshop/skill_launcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,19 @@

from ovos_workshop.skills.active import ActiveSkill
from ovos_workshop.skills.auto_translatable import UniversalSkill, UniversalFallback
from ovos_workshop.skills.base import BaseSkill
from ovos_workshop.skills.common_play import OVOSCommonPlaybackSkill
from ovos_workshop.skills.common_query_skill import CommonQuerySkill
from ovos_workshop.skills.fallback import FallbackSkill
from ovos_workshop.skills.mycroft_skill import MycroftSkill
from ovos_workshop.skills.ovos import OVOSSkill, OVOSFallbackSkill
from ovos_workshop.skills.ovos import OVOSSkill

SKILL_BASE_CLASSES = [
BaseSkill, MycroftSkill, OVOSSkill, OVOSFallbackSkill,
OVOSCommonPlaybackSkill, OVOSFallbackSkill, CommonQuerySkill, ActiveSkill,
OVOSSkill, OVOSCommonPlaybackSkill, CommonQuerySkill, ActiveSkill,
FallbackSkill, UniversalSkill, UniversalFallback
]

SKILL_MAIN_MODULE = '__init__.py'



def remove_submodule_refs(module_name: str):
"""
Ensure submodules are reloaded by removing the refs from sys.modules.
Expand Down Expand Up @@ -85,13 +81,13 @@ def load_skill_module(path: str, skill_id: str) -> ModuleType:

def get_skill_class(skill_module: ModuleType) -> Optional[callable]:
"""
Find MycroftSkill based class in skill module.
Find OVOSSkill based class in skill module.

Arguments:
skill_module (module): module to search for Skill class

Returns:
(MycroftSkill): Found subclass of MycroftSkill or None.
(OVOSSkill): Found subclass of OVOSSkill or None.
Comment on lines +84 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider simplifying the get_skill_class function

The docstring has been updated to reflect the transition to OVOSSkill, which is good. However, the function's implementation still uses a generic approach to find skill classes.

Consider simplifying this function to specifically look for OVOSSkill subclasses, as this could potentially improve performance and readability. Here's a suggested implementation:

def get_skill_class(skill_module: ModuleType) -> Optional[Type[OVOSSkill]]:
    """
    Find OVOSSkill based class in skill module.

    Arguments:
        skill_module (module): module to search for Skill class

    Returns:
        (Type[OVOSSkill]): Found subclass of OVOSSkill or None.
    """
    if not skill_module:
        raise ValueError("Expected module and got None")
    if callable(skill_module):
        return skill_module

    for name, obj in skill_module.__dict__.items():
        if isclass(obj) and issubclass(obj, OVOSSkill) and obj is not OVOSSkill:
            return obj

    return None

This implementation is more straightforward and directly aligns with the OVOS-specific structure.

"""
if not skill_module:
raise ValueError("Expected module and got None")
Expand Down Expand Up @@ -156,7 +152,7 @@ def __init__(self, bus: MessageBusClient,
self._loaded = None
self.load_attempted = False
self.last_loaded = 0
self.instance: Optional[BaseSkill] = None
self.instance: Optional[OVOSSkill] = None
self.active = True
self._watchdog = None
self.config = Configuration()
Expand Down
7 changes: 4 additions & 3 deletions ovos_workshop/skills/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from ovos_workshop.decorators.layers import IntentLayers
from ovos_workshop.skills.ovos import OVOSSkill, OVOSFallbackSkill
from ovos_workshop.skills.ovos import OVOSSkill
from ovos_workshop.skills.idle_display_skill import IdleDisplaySkill
from ovos_workshop.skills.mycroft_skill import MycroftSkill

from ovos_workshop.skills.fallback import FallbackSkill
from ovos_workshop.skills.common_query_skill import CommonQuerySkill
from ovos_workshop.skills.common_play import OVOSCommonPlaybackSkill
4 changes: 2 additions & 2 deletions ovos_workshop/skills/active.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ def bind(self, bus):
super(ActiveSkill, self).bind(bus)
if bus:
""" insert skill in active skill list on load """
self.make_active()
self.activate()
JarbasAl marked this conversation as resolved.
Show resolved Hide resolved

def handle_skill_deactivated(self, message=None):
"""
skill is always in active skill list, ie, converse is always called
"""
self.make_active()
self.activate()


6 changes: 3 additions & 3 deletions ovos_workshop/skills/auto_translatable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from ovos_utils.log import LOG
from ovos_workshop.resource_files import SkillResources
from ovos_workshop.skills.common_query_skill import CommonQuerySkill
from ovos_workshop.skills.fallback import FallbackSkillV2
from ovos_workshop.skills.fallback import FallbackSkill
from ovos_workshop.skills.ovos import OVOSSkill


Expand Down Expand Up @@ -311,7 +311,7 @@ def _handle_converse_request(self, message: Message):
super()._handle_converse_request(message)


class UniversalFallback(UniversalSkill, FallbackSkillV2):
class UniversalFallback(UniversalSkill, FallbackSkill):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Remaining reference to FallbackSkillV2 found in tests.

A reference to FallbackSkillV2 was found in the test file test/unittests/skills/test_fallback_skill.py:

  • class TestFallbackSkillV2(TestCase):

Please update or remove this reference to ensure all parts of the codebase are consistent with the recent changes.

🔗 Analysis chain

LGTM! Verify impact on dependent code.

The changes to the UniversalFallback class, including updating the inheritance from FallbackSkillV2 to FallbackSkill and modifying the register_fallback method, appear to be part of the larger refactoring effort to remove Mycroft components. These changes are consistent and should simplify the skill hierarchy.

To ensure these changes don't introduce any unintended side effects, please run the following script to check for any remaining references to FallbackSkillV2 in the codebase:

If the script returns any results, those occurrences may need to be updated to maintain consistency with these changes.

Also applies to: 376-376

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to FallbackSkillV2
rg --type python "FallbackSkillV2"

Length of output: 65

Script:

#!/bin/bash
# Search for any remaining references to FallbackSkillV2 using the correct file type
rg --type py "FallbackSkillV2"

Length of output: 112

"""
Fallback Skill that auto translates input/output from any language.
Expand Down Expand Up @@ -373,7 +373,7 @@ def register_fallback(self, handler, priority: int):
- `create_universal_fallback_handler` method for creating universal fallback handlers.
"""
handler = self.create_universal_fallback_handler(handler)
FallbackSkillV2.register_fallback(self, handler, priority)
FallbackSkill.register_fallback(self, handler, priority)


class UniversalCommonQuerySkill(UniversalSkill, CommonQuerySkill, ABC):
Expand Down
17 changes: 0 additions & 17 deletions ovos_workshop/skills/base.py
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion ovos_workshop/skills/common_play.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# backwards compat imports, do not delete, skills import from here
from ovos_workshop.decorators.ocp import ocp_play, ocp_next, ocp_pause, ocp_resume, ocp_search, \
ocp_previous, ocp_featured_media
from ovos_workshop.backwards_compat import MediaType, MediaState, MatchConfidence, \
from ovos_utils.ocp import MediaType, MediaState, MatchConfidence, \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unused imports

The static analysis tool has correctly identified that MediaState and MatchConfidence are imported but not used in the code. To improve code cleanliness and reduce potential confusion, it's recommended to remove these unused imports.

Apply this diff to remove the unused imports:

-from ovos_utils.ocp import MediaType, MediaState, MatchConfidence, \
+from ovos_utils.ocp import MediaType, \
    PlaybackType, PlaybackMode, PlayerState, LoopState, TrackState, Playlist, PluginStream, MediaEntry
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
from ovos_utils.ocp import MediaType, MediaState, MatchConfidence, \
from ovos_utils.ocp import MediaType, \
PlaybackType, PlaybackMode, PlayerState, LoopState, TrackState, Playlist, PluginStream, MediaEntry
🧰 Tools
🪛 Ruff

16-16: ovos_utils.ocp.MediaState imported but unused

Remove unused import

(F401)

16-16: ovos_utils.ocp.MatchConfidence imported but unused

Remove unused import

(F401)

PlaybackType, PlaybackMode, PlayerState, LoopState, TrackState, Playlist, PluginStream, MediaEntry


Expand Down
16 changes: 0 additions & 16 deletions ovos_workshop/skills/common_query_skill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
from ovos_utils.file_utils import resolve_resource_file
from ovos_utils.log import LOG, log_deprecation

from ovos_workshop.decorators.compat import backwards_compat
from ovos_workshop.skills.ovos import OVOSSkill


Expand Down Expand Up @@ -233,21 +232,6 @@ def __calc_confidence(self, match: str, phrase: str, level: CQSMatchLevel,

return confidence

def __handle_query_classic(self, message):
"""
does not perform self.speak, < 0.0.8 this is done by core itself
"""
if message.data["skill_id"] != self.skill_id:
# Not for this skill!
return
self.activate()
phrase = message.data["phrase"]
data = message.data.get("callback_data") or {}
# Invoke derived class to provide playback data
self.CQS_action(phrase, data)

@backwards_compat(classic_core=__handle_query_classic,
pre_008=__handle_query_classic)
def __handle_query_action(self, message: Message):
"""
If this skill's response was spoken to the user, this method is called.
Expand Down
4 changes: 0 additions & 4 deletions ovos_workshop/skills/decorators/__init__.py
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions ovos_workshop/skills/decorators/converse.py
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions ovos_workshop/skills/decorators/fallback_handler.py
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions ovos_workshop/skills/decorators/killable.py
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions ovos_workshop/skills/decorators/layers.py
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions ovos_workshop/skills/decorators/ocp.py
View file
Open in desktop

This file was deleted.

Loading
Loading