fix: remove mycroft-bus-client compat #304
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ovos_utils/events.py (1)
6-6: Consider updating docstrings to reflect the new message type.The type change from mycroft-bus-client's Message to FakeMessage should be documented in relevant docstrings for clarity.
Example updates for docstrings:
- message (Message): Intent result message + message (FakeMessage): Intent result message from ovos_utils.fakebus
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/events.py(1 hunks)
🔇 Additional comments (1)
ovos_utils/events.py (1)
6-6: LGTM! Verify FakeMessage compatibility.
The import change from Message to FakeMessage as Message maintains backward compatibility while removing mycroft-bus-client dependency. Let's verify the full compatibility of the new message type.
✅ Verification successful
Import change is safe and compatible
The verification confirms that FakeMessage is a fully compatible drop-in replacement for Message:
FakeMessageimplements all required attributes (data,context,msg_type) used throughout the codebase- It's designed specifically to be API compatible, as evidenced by its metaclass
_MutableMessageinfakebus.py - The test suite already uses this message type successfully
- No direct imports of
mycroft_bus_client.message.Messagewere found that would need updating
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that FakeMessage provides all required functionality
# Check for any direct Message imports that might need updating
rg "from mycroft_bus_client.message import Message"
# Check for any Message type hints that might need updating
rg "Message[^a-zA-Z]"
# Look for potential message attribute access that might differ
rg "\bmessage\.(data|context|msg_type)\b"
Length of output: 4548
JarbasAl
force-pushed
the
JarbasAl-patch-1
branch
from
adf6cee to
7177e0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ovos_utils/fakebus.py (1)
Line range hint
144-154: Improve metaclass implementation consistency.The variable name
_MycroftMessageis misleading since we're importing fromovos_bus_client. Consider renaming for clarity.def __instancecheck__(self, instance): try: - from ovos_bus_client.message import Message as _MycroftMessage - if isinstance(instance, _MycroftMessage): + from ovos_bus_client.message import Message as _OVOSMessage + if isinstance(instance, _OVOSMessage): return True except ImportError: pass return super().__instancecheck__(instance)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/fakebus.py(1 hunks)
🔇 Additional comments (2)
ovos_utils/fakebus.py (2)
Line range hint 1-324: Overall implementation looks good!
The changes successfully remove the mycroft-bus-client compatibility while maintaining a robust fake message bus implementation for testing purposes. The error handling is consistent and well-documented throughout the code.
189-190: Verify the import fallback behavior.
The simplified fallback looks good, but we should ensure proper error handling for environments where ovos_bus_client is not available.
✅ Verification successful
Import fallback behavior is safe to simplify
The simplified fallback in FakeMessage.__new__ is safe because:
FakeMessageis only used internally withinovos_utilsand imported as an alias forMessageinevents.py- The only
isinstancecheck in the codebase is inunmunge_messagewhich uses the aliasedMessagetype - The usage is for testing purposes and doesn't rely on mycroft-bus-client compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any direct imports of FakeMessage in the codebase
# that might be affected by the removal of mycroft-bus-client fallback
# Search for direct FakeMessage imports
rg "from ovos_utils.fakebus import FakeMessage"
# Search for potential message type checks that might rely on mycroft-bus-client
ast-grep --pattern 'isinstance($_, Message)'
Length of output: 263
Script:
#!/bin/bash
# Let's check the implementation in events.py to understand the usage context
cat ovos_utils/events.py
# Also check if there are any test files using FakeMessage
fd "test.*\.py" --exec rg "FakeMessage"
Length of output: 15342
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ovos_utils/fakebus.py (2)
Line range hint
171-182: Consider simplifying the metaclass for consistency.Since we're removing
mycroft-bus-clientcompatibility, the_MutableMessagemetaclass could be simplified to only check forovos_bus_client.Message. This would make the code more consistent with the changes in__new__.def __instancecheck__(self, instance): try: - from ovos_bus_client.message import Message as _MycroftMessage - if isinstance(instance, _MycroftMessage): + from ovos_bus_client.message import Message + if isinstance(instance, Message): return True except ImportError: pass return super().__instancecheck__(instance)
Line range hint
183-188: Add type hints to improve code maintainability.Consider adding type hints to the
FakeMessageclass methods to improve code maintainability and IDE support.-class FakeMessage(metaclass=_MutableMessage): +from typing import Optional, Dict, Any + +class FakeMessage(metaclass=_MutableMessage): """ fake Message object to allow usage with FakeBus without ovos-bus-client installed""" - def __new__(cls, *args, **kwargs): + def __new__(cls, *args: Any, **kwargs: Any) -> "FakeMessage":
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
ovos_utils/events.py(1 hunks)ovos_utils/fakebus.py(1 hunks)ovos_utils/messagebus.py(0 hunks)test/unittests/test_event_scheduler.py(2 hunks)test/unittests/test_events.py(1 hunks)
💤 Files with no reviewable changes (1)
- ovos_utils/messagebus.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ovos_utils/events.py
- test/unittests/test_events.py
🔇 Additional comments (5)
test/unittests/test_event_scheduler.py (3)
8-8: LGTM! Import statement updated correctly.
The import statement has been properly updated to reflect the new location of FakeBus in ovos_utils.fakebus, aligning with the architectural changes removing mycroft-bus-client compatibility.
119-119: LGTM! Event cleanup verification is correct.
The assertion properly verifies that the event scheduler cleans up registered events during shutdown by checking the internal event emitter state.
Line range hint 1-119: Verify test coverage after architectural changes.
The test suite appears comprehensive, but let's verify that all critical paths are still covered after the removal of mycroft-bus-client compatibility.
✅ Verification successful
Let me gather more information about the test coverage and event scheduler implementation to ensure completeness.
Let me check the actual event scheduler implementation and its test coverage with a different approach.
Test coverage remains comprehensive after architectural changes
The test suite adequately covers the essential functionality after the migration from mycroft-bus-client:
- All mycroft-bus-client references have been properly replaced with ovos-bus-client
- Test cases cover core operations: creation, scheduling, removal, and shutdown
- FakeBus implementation is properly used for testing bus interactions
- Event scheduling and interface tests remain intact and comprehensive
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to mycroft-bus-client and verify test coverage
# Check for any remaining mycroft-bus-client references
echo "Checking for remaining mycroft-bus-client references..."
rg "mycroft.bus" || echo "No mycroft.bus references found"
rg "mycroft_bus_client" || echo "No mycroft_bus_client references found"
# Look for any TODO or FIXME comments related to bus client
echo "Checking for TODO/FIXME comments related to bus client..."
rg -i "todo.*bus|fixme.*bus" || echo "No TODO/FIXME comments found"
# Check test file imports to ensure consistency
echo "Checking test file imports..."
rg "^from.*import.*Bus" "test/" || echo "No Bus imports found in tests"
Length of output: 966
Script:
#!/bin/bash
# Check event scheduler implementation and related tests
echo "Checking EventScheduler implementation..."
ast-grep --pattern 'class EventScheduler'
echo "Checking for event-related test files..."
fd "test.*event.*\.py" test/
echo "Checking event-related imports in tests..."
rg "from.*event" test/
echo "Checking message bus related tests..."
rg "class.*Test.*Bus" test/
Length of output: 1514
Script:
#!/bin/bash
# Check EventScheduler implementation and test coverage
echo "Checking EventScheduler class implementation..."
rg -A 10 "class EventScheduler"
echo "Checking test methods in event scheduler tests..."
rg "def test_" test/unittests/test_event_scheduler.py
echo "Checking bus client imports and usage..."
rg "ovos_bus_client" test/unittests/test_event_scheduler.py -A 2
Length of output: 1495
ovos_utils/fakebus.py (2)
189-190: LGTM! The fallback implementation is correct.
The simplified fallback implementation in __new__ correctly creates a new instance when ovos_bus_client is not available.
Line range hint 9-17: Consider removing unused function.
The dig_for_message function appears to be unused in this file. If it's not used elsewhere in the codebase, consider removing it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #304 +/- ##
==========================================
+ Coverage 44.74% 44.87% +0.12%
==========================================
Files 33 33
Lines 3426 3412 -14
==========================================
- Hits 1533 1531 -2
+ Misses 1893 1881 -12 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/unittests/test_event_scheduler.py (1)
119-119: LGTM! Consider enhancing assertion readability.The assertion correctly verifies event cleanup through the bus emitter's event dictionary.
Consider improving readability by extracting the event lookup into a variable:
- self.assertTrue(len(es.bus.ee._events.get('id:f', [])) == 0) + event_handlers = es.bus.ee._events.get('id:f', []) + self.assertTrue(len(event_handlers) == 0)ovos_utils/fakebus.py (2)
189-190: Consider adding a warning log for missing ovos_bus_clientWhen
ovos_bus_clientimport fails, the code silently falls back toFakeMessage. Consider adding a warning log to help diagnose potential configuration issues.try: # most common case from ovos_bus_client import Message as _M return _M(*args, **kwargs) except ImportError: + LOG.warning("ovos_bus_client not found, falling back to FakeMessage") pass return super().__new__(cls)
Line range hint
155-166: Consider caching the Message import resultThe
__instancecheck__method attempts to importMessageon every call. Consider caching the import result at the class level to improve performance.class _MutableMessage(type): """ To override isinstance checks we need to use a metaclass """ + _mycroft_message = None + + @classmethod + def _get_mycroft_message(cls): + if cls._mycroft_message is None: + try: + from ovos_bus_client.message import Message as _MycroftMessage + cls._mycroft_message = _MycroftMessage + except ImportError: + cls._mycroft_message = False + return cls._mycroft_message def __instancecheck__(self, instance): - try: - from ovos_bus_client.message import Message as _MycroftMessage - if isinstance(instance, _MycroftMessage): - return True - except ImportError: - pass + message_class = self._get_mycroft_message() + if message_class and isinstance(instance, message_class): + return True return super().__instancecheck__(instance)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
ovos_utils/events.py(1 hunks)ovos_utils/fakebus.py(2 hunks)ovos_utils/messagebus.py(0 hunks)test/unittests/test_event_scheduler.py(2 hunks)test/unittests/test_events.py(1 hunks)
💤 Files with no reviewable changes (1)
- ovos_utils/messagebus.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ovos_utils/events.py
- test/unittests/test_events.py
🔇 Additional comments (3)
test/unittests/test_event_scheduler.py (1)
8-8: LGTM! Import path updated correctly.
The import statement has been properly updated to use the new path, which aligns with the removal of mycroft-bus-client compatibility.
ovos_utils/fakebus.py (2)
63-63: LGTM! Simplified message deserialization
The change to use FakeMessage.deserialize directly makes the code cleaner while maintaining functionality.
Line range hint 1-1: Verify complete removal of mycroft-bus-client references
Let's ensure all references to mycroft-bus-client have been removed from the codebase.
✅ Verification successful
All mycroft-bus-client references have been properly removed
Based on the comprehensive verification:
- No mycroft-bus-client dependency in requirements.txt
- No direct imports from mycroft.bus
- MessageBusClient references in docstrings are generic and not tied to mycroft-bus-client
- The bus client functionality has been replaced with ovos-bus-client (evident from imports)
- No remaining mycroft-bus-client references in setup.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to mycroft-bus-client
# Search for mycroft-bus-client in Python files
echo "Checking for mycroft-bus-client references in Python files..."
rg -l "mycroft[_-]bus[_-]client" --type py
# Search for mycroft-bus-client in requirements files
echo "Checking for mycroft-bus-client in requirements..."
rg -l "mycroft[_-]bus[_-]client" -g "requirements*.txt"
# Search for mycroft-bus-client in setup files
echo "Checking for mycroft-bus-client in setup files..."
rg -l "mycroft[_-]bus[_-]client" -g "setup.py"
Length of output: 502
Script:
#!/bin/bash
# Let's try a more comprehensive search with different patterns and include context
# Search for any mentions of mycroft bus client with context
echo "Checking for 'mycroft bus client' mentions..."
rg -i "mycroft.*bus.*client" -A 2 -B 2
# Search for MessageBusClient class references
echo "Checking for MessageBusClient references..."
rg "MessageBusClient" -A 2 -B 2
# Search for any bus client imports
echo "Checking for bus client imports..."
ast-grep --pattern 'from mycroft.bus $$$'
# Check package dependencies
echo "Checking package dependencies..."
cat setup.py
Length of output: 4586
Script:
#!/bin/bash
# Let's check the requirements files and docstrings for any remaining references
# Check requirements files
echo "Checking requirements.txt..."
cat requirements/requirements.txt
# Check for mycroft bus in docstrings
echo "Checking docstrings for mycroft bus mentions..."
rg -i "mycroft.*bus" "*.py" -A 2 -B 2 --type py
# Check specific files that had MessageBusClient references
echo "Checking content of files with MessageBusClient..."
cat ovos_utils/events.py
cat ovos_utils/process_utils.py
cat ovos_utils/gui.py
Length of output: 32656
Summary by CodeRabbit
New Features
ovos_bus_client.mycroft_bus_client.Bug Fixes
__instancecheck__method to improve performance.Refactor
FakeMessageclass to eliminate unnecessary fallbacks and deprecated warnings.