refactor: improve code organization and add type hints#29
Conversation
- Move data models (AggregatedData, EMGDataSingle) from core.py to types.py - Create utils module for GATT utility functions - Add comprehensive type hints throughout the codebase - Add detailed docstrings to classes and methods - Fix FirmwareInfo.to_dict() bug (unlock_pose field) - Fix Vibrate2 command byte shifting - Extract helper methods to reduce code duplication - Improve error handling and validation - Update imports in __init__.py All changes maintain backward compatibility.
WalkthroughReorganized public exports to move Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MyoClient
participant Bleak as BleakClient
participant Utils as myo.utils.gatt_char_to_dict
rect rgba(214,234,248,0.5)
note right of MyoClient: Typed async API & notify routing
end
Caller->>MyoClient: with_device(mac, aggregate_*)
MyoClient->>Bleak: connect() / get_services()
Bleak-->>MyoClient: characteristics list
loop each characteristic
MyoClient->>Utils: gatt_char_to_dict(Bleak, char)
Utils-->>MyoClient: dict(name, uuid, properties, value?)
end
MyoClient->>MyoClient: derive handles (EMG/IMU/classifier)
Caller->>MyoClient: start() / start_notify(handles)
MyoClient->>Bleak: start_notify(handle, notify_callback)
Bleak->>MyoClient: notify_callback(sender, data)
MyoClient->>MyoClient: route/convert data -> (FVData/IMUData/EMGData / AggregatedData)
MyoClient-->>Caller: on_data / on_aggregated_data callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (3)myo/utils.py (2)
myo/commands.py (1)
myo/core.py (3)
Comment |
The test was expecting the old buggy behavior where unlock_pose returned True (which was actually has_custom_classifier value). Now it correctly expects 'DOUBLE_TAP' which is the actual unlock pose name parsed from the firmware info blob.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
- Coverage 85.63% 80.23% -5.41%
==========================================
Files 7 8 +1
Lines 376 425 +49
Branches 32 38 +6
==========================================
+ Hits 322 341 +19
- Misses 50 80 +30
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
myo/__init__.py(1 hunks)myo/commands.py(6 hunks)myo/core.py(5 hunks)myo/types.py(2 hunks)myo/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
myo/utils.py (2)
myo/profile.py (1)
Handle(101-123)myo/types.py (11)
FirmwareInfo(143-167)FirmwareVersion(171-180)to_dict(55-66)to_dict(106-107)to_dict(124-125)to_dict(158-167)to_dict(203-204)to_dict(230-235)to_dict(269-279)to_dict(359-360)to_dict(375-376)
myo/__init__.py (2)
myo/core.py (2)
Myo(50-234)MyoClient(237-701)myo/types.py (5)
AggregatedData(346-360)ClassifierEvent(27-66)ClassifierMode(81-83)EMGData(95-107)EMGDataSingle(363-376)
myo/commands.py (1)
myo/types.py (7)
ClassifierMode(81-83)EMGMode(131-138)IMUMode(240-245)SleepMode(308-310)UnlockType(319-322)UserActionType(326-327)VibrationType(331-335)
myo/core.py (4)
myo/commands.py (10)
Command(19-37)DeepSleep(83-89)LED(93-116)SetMode(41-65)SetSleepMode(161-171)Unlock(175-185)UserAction(188-198)Vibrate(69-79)Vibrate2(120-157)data(30-34)myo/profile.py (2)
GATTProfile(10-97)Handle(101-123)myo/types.py (18)
AggregatedData(346-360)ClassifierMode(81-83)EMGData(95-107)EMGDataSingle(363-376)EMGMode(131-138)FVData(112-125)IMUData(192-235)IMUMode(240-245)MotionEvent(250-279)SleepMode(308-310)VibrationType(331-335)json(52-53)json(103-104)json(121-122)json(227-228)json(266-267)json(356-357)json(372-373)myo/utils.py (1)
gatt_char_to_dict(19-57)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
myo/utils.py (2)
44-53: Fix battery-level decoding
BleakClient.read_gatt_charreturns abytearray; callingordon it raisesTypeError, so the battery branch currently crashes. Please extract the first byte (and allow for empty payloads) instead.- elif char_name == Handle.BATTERY_LEVEL.name: - value = ord(blob) + elif char_name == Handle.BATTERY_LEVEL.name: + value = blob[0] if blob else None
55-56: Preserve falsy characteristic valuesThe
if value:guard discards legitimate readings such as a 0% battery level. Only skip assignment when the value isNone.- if value: + if value is not None: cd["value"] = value
🧹 Nitpick comments (2)
myo/core.py (2)
308-310: Consider removing unnecessary None check.The
BleakClientconstructor won't returnNoneaccording to the Bleak library documentation. It will either return a client instance or raise an exception. This None check may be unnecessary defensive code.
377-379: Consider using ABC for interface methods.The callback methods (
on_classifier_event,on_aggregated_data,on_emg_data, etc.) are meant to be overridden by subclasses. Consider usingabc.ABCand@abstractmethoddecorators for clearer intent and compile-time enforcement.Example:
+from abc import ABC, abstractmethod + -class MyoClient: +class MyoClient(ABC): ... + @abstractmethod async def on_classifier_event(self, ce: ClassifierEvent) -> None: """Handle classifier events. Override in subclasses.""" raise NotImplementedError()Also applies to: 401-412, 414-421, 423-430, 432-439, 441-448, 450-457
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
myo/__init__.py(1 hunks)myo/commands.py(6 hunks)myo/core.py(5 hunks)myo/types.py(2 hunks)myo/utils.py(1 hunks)tests/test_myo/test_types.py(1 hunks)
🔇 Additional comments (7)
myo/core.py (7)
10-11: LGTM! Good import organization.The addition of type hints imports and moving
gatt_char_to_dictto the utils module improves code organization.Also applies to: 45-45
55-63: LGTM! Proper initialization and validation.The explicit type hints and None-guard in the
deviceproperty with a clear error message are excellent defensive programming practices.
66-114: LGTM! Robust discovery methods.The factory methods have proper type hints, comprehensive docstrings, and good error handling with informative logging.
143-166: Excellent input validation!The comprehensive validation of LED payload (length and value ranges) with clear error messages is a great defensive programming practice that prevents invalid data from reaching the device.
588-614: Excellent refactoring!Extracting these helper methods (
_get_emg_handles,_get_imu_handles,_get_classifier_handles) significantly improves code maintainability by:
- Eliminating duplication between
start()andstop()methods- Centralizing the logic for determining which handles to subscribe/unsubscribe
- Making the code more readable with clear method names
616-638: LGTM! Robust notification management.The
start()andstop()methods properly validate state before operations and make excellent use of the helper methods for clean, maintainable code.Also applies to: 646-671
297-301: LGTM! Consistent and thorough validation.The consistent validation pattern across all methods ensures that:
- Operations requiring connection check
self._client is None- Operations requiring setup check mode state
- Error messages are clear and actionable
This significantly improves API usability and debugging experience.
Also applies to: 315-319, 366-375, 504-520, 527-531, 547-549, 618-621, 640-644, 648-651, 673-677, 679-683, 685-689, 691-695, 697-701
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(2 hunks)myo/core.py(5 hunks)myo/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
myo/utils.py (2)
myo/profile.py (1)
Handle(101-123)myo/types.py (11)
FirmwareInfo(143-167)FirmwareVersion(171-180)to_dict(55-66)to_dict(106-107)to_dict(124-125)to_dict(158-167)to_dict(203-204)to_dict(230-235)to_dict(269-279)to_dict(359-360)to_dict(375-376)
myo/core.py (4)
myo/types.py (17)
json(52-53)json(103-104)json(121-122)json(227-228)json(266-267)json(356-357)json(372-373)AggregatedData(346-360)ClassifierMode(81-83)EMGData(95-107)EMGDataSingle(363-376)EMGMode(131-138)FVData(112-125)IMUData(192-235)IMUMode(240-245)SleepMode(308-310)VibrationType(331-335)myo/commands.py (10)
Command(19-37)DeepSleep(83-89)LED(93-116)SetMode(41-65)SetSleepMode(161-171)Unlock(175-185)UserAction(188-198)Vibrate(69-79)Vibrate2(120-157)data(30-34)myo/profile.py (2)
GATTProfile(10-97)Handle(101-123)myo/utils.py (1)
gatt_char_to_dict(20-58)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
myo/core.py (3)
157-162: Consider using a helper for RGB validation.The validation logic is correct but duplicated for logo and line. Since this is "Chill" mode, consider extracting a helper if this pattern repeats elsewhere.
For example:
def _validate_rgb(values: list[int], name: str) -> None: if len(values) != 3: raise ValueError(f"{name} must have 3 values (R, G, B)") if any(not isinstance(v, int) or not 0 <= v <= 255 for v in values): raise ValueError(f"{name} values must be integers 0-255")
276-283: Consider adding timeout or retry limit to device discovery.The infinite loop will retry forever if the device is unavailable. For better UX, consider adding a timeout or max retry count with appropriate logging.
Example:
max_retries = 30 # 30 seconds for attempt in range(max_retries): if mac and mac != "": self.m = await Myo.with_mac(mac) else: self.m = await Myo.with_uuid() if self.m is not None: break logger.warning("Device not found, retrying... (%d/%d)", attempt + 1, max_retries) await asyncio.sleep(1) else: raise TimeoutError("Failed to discover device after 30 seconds")
306-310: Remove unnecessary None check; consider exception handling.The
BleakClient()constructor never returnsNone, so the check at lines 307-308 is dead code and can be removed.Additionally, if
connect()raises an exception,self._clientis left assigned but not connected. Consider wrapping in try/except to clean up state on failure:self._client = BleakClient(self.device) - if self._client is None: - raise RuntimeError("Failed to create BLE client") - - await self._client.connect() + try: + await self._client.connect() + except Exception: + self._client = None + raise logger.info("connected to %s: %s", self.device.name, self.device.address)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks)myo/core.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🧬 Code graph analysis (1)
myo/core.py (4)
myo/types.py (19)
json(52-53)json(103-104)json(121-122)json(227-228)json(266-267)json(356-357)json(372-373)AggregatedData(346-360)ClassifierEvent(27-66)ClassifierMode(81-83)EMGData(95-107)EMGDataSingle(363-376)EMGMode(131-138)FVData(112-125)IMUData(192-235)IMUMode(240-245)MotionEvent(250-279)SleepMode(308-310)VibrationType(331-335)myo/commands.py (9)
Command(19-37)LED(93-116)SetMode(41-65)SetSleepMode(161-171)Unlock(175-185)UserAction(188-198)Vibrate(69-79)Vibrate2(120-157)data(30-34)myo/profile.py (2)
GATTProfile(10-97)Handle(101-123)myo/utils.py (1)
gatt_char_to_dict(20-58)
🔇 Additional comments (15)
myo/core.py (15)
57-65: LGTM: Clean initialization and property access.The type hints and validation ensure callers must use
with_mac()orwith_uuid()before accessing the device.
68-92: LGTM: MAC-based discovery is correctly implemented.The case-insensitive comparison and error handling are appropriate.
103-107: LGTM: UUID discovery now handles None service_uuids safely.The guard
uuids = adv.service_uuids or []prevents the TypeError flagged in the previous review.
116-129: LGTM: Battery level conversion now handles bytearray correctly.The fix replaces
ord(val)withint.from_bytes(val, "little")and adds validation, resolving the TypeError from the previous review.
203-217: LGTM: AttributeError handling is now defensive.The use of
getattr(client, "is_connected", None)in the debug log prevents re-raising the AttributeError, addressing the previous review concern.
131-232: LGTM: Command delegation methods are well-documented and typed.The type hints and docstrings provide clear interfaces for all command methods.
238-255: LGTM: Well-documented initialization with clear aggregation flags.The docstrings and type hints clarify the purpose of
aggregate_allandaggregate_emg.
319-328: LGTM: Graceful handling of already-disconnected state.The early return for
Noneclient prevents double-disconnect errors.
589-615: LGTM: Helper methods eliminate duplication.Extracting these handle-selection methods makes
start()andstop()much cleaner and reduces the risk of inconsistency.
462-503: LGTM: Notification routing correctly handles aggregation modes.The conditional routing based on
aggregate_allandaggregate_emgflags ensures data flows to the appropriate handlers.
564-572: LGTM: Mode enforcement for aggregate_all is appropriate.Forcing specific modes when
aggregate_all=Trueensures the aggregation logic receives the required FVData and IMUData streams.
617-672: LGTM: Symmetric start/stop with proper mode validation.The validation ensures
setup()has been called beforestart()orstop(), and the helper methods keep the logic clean.
375-460: LGTM: Well-documented abstract interface for event handlers.The docstrings clearly explain what data each handler receives, and the note about EMG vs FV/IMU intervals (line 404-405) is particularly helpful.
295-373: LGTM: Client wrapper methods properly validate and delegate.All methods check connection state before delegating to the underlying
Myoinstance.
380-396: LGTM: Thread-safe aggregation with proper synchronization.The lock protects the aggregation state, and the logic correctly waits for both FVData and IMUData before triggering the aggregated handler.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @iomz. * #29 (comment) The following files were modified: * `myo/commands.py` * `myo/core.py` * `myo/types.py` * `myo/utils.py`
Docstrings generation was requested by @iomz. * #29 (comment) The following files were modified: * `myo/commands.py` * `myo/core.py` * `myo/types.py` * `myo/utils.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
myo/core.py (2)
126-138: Past issue resolved.The UUID matching now safely handles
Noneservice_uuids by defaulting to an empty list, preventing the TypeError mentioned in previous reviews.
148-164: Past issue resolved.The battery level reading now correctly uses
int.from_bytes()with proper empty-payload validation, fixing the TypeError from the previousord(val)usage.
🧹 Nitpick comments (3)
myo/utils.py (1)
41-68: Past review issues resolved; consider optional defensive error handling.The fixes for battery level decoding (
int.from_byteswith empty-blob guard) and the falsy-value check (is not None) correctly address the prior critical and major issues.For additional robustness, consider wrapping UTF-8 decoding and struct unpacks in try-except blocks to handle malformed payloads gracefully, though this is optional given the controlled BLE environment.
Example defensive pattern:
try: value = blob.decode("utf-8") except UnicodeDecodeError: logger.warning("Invalid UTF-8 in MANUFACTURER_NAME_STRING") value = binascii.b2a_hex(blob).decode("utf-8")myo/commands.py (1)
86-86: Wrap long docstring lines to meet style guide.Three docstring lines exceed the 120-character limit. Consider wrapping them for consistency with the project's linting rules.
Based on static analysis hints.
Also applies to: 127-127, 195-195
myo/core.py (1)
257-263: Logging safety improved; consider adding inline comment.The use of
getattr(client, "is_connected", None)correctly prevents the logger itself from raising an AttributeError. The docstring mentions the exception handling, but an inline comment explaining this is a known Bleak quirk would improve maintainability.Example:
except AttributeError: # Known Bleak issue: is_connected may be missing on certain backends logger.debug(...)Based on past review feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
myo/commands.py(6 hunks)myo/core.py(5 hunks)myo/types.py(2 hunks)myo/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
myo/utils.py (2)
myo/profile.py (1)
Handle(101-123)myo/types.py (11)
FirmwareInfo(143-180)FirmwareVersion(184-193)to_dict(55-66)to_dict(106-107)to_dict(124-125)to_dict(158-180)to_dict(216-217)to_dict(243-248)to_dict(282-292)to_dict(393-401)to_dict(436-443)
myo/core.py (5)
myo/types.py (19)
json(52-53)json(103-104)json(121-122)json(240-241)json(279-280)json(382-391)json(425-434)AggregatedData(359-401)ClassifierEvent(27-66)ClassifierMode(81-83)EMGData(95-107)EMGDataSingle(404-443)EMGMode(131-138)FVData(112-125)IMUData(205-248)IMUMode(253-258)MotionEvent(263-292)SleepMode(321-323)VibrationType(344-348)myo/commands.py (10)
Command(19-53)DeepSleep(118-129)LED(133-156)SetMode(57-94)SetSleepMode(207-223)Unlock(227-243)UserAction(246-262)Vibrate(98-114)Vibrate2(160-203)data(35-44)myo/profile.py (2)
GATTProfile(10-97)Handle(101-123)myo/utils.py (1)
gatt_char_to_dict(20-68)examples/sample_client.py (6)
on_classifier_event(24-26)on_aggregated_data(28-29)on_emg_data(31-33)on_fv_data(35-37)on_imu_data(39-41)on_motion_event(43-44)
myo/commands.py (1)
myo/types.py (7)
ClassifierMode(81-83)EMGMode(131-138)IMUMode(253-258)SleepMode(321-323)UnlockType(332-335)UserActionType(339-340)VibrationType(344-348)
🪛 GitHub Actions: build
myo/commands.py
[error] 86-86: E501 Line too long (138 > 120)
🪛 GitHub Check: ruff
myo/core.py
[failure] 188-188: Ruff (E501)
myo/core.py:188:121: E501 Line too long (121 > 120)
[failure] 171-171: Ruff (E501)
myo/core.py:171:121: E501 Line too long (139 > 120)
[failure] 134-134: Ruff (E501)
myo/core.py:134:121: E501 Line too long (152 > 120)
[failure] 61-61: Ruff (E501)
myo/core.py:61:121: E501 Line too long (124 > 120)
[failure] 255-255: Ruff (E501)
myo/core.py:255:121: E501 Line too long (211 > 120)
[failure] 247-247: Ruff (E501)
myo/core.py:247:121: E501 Line too long (132 > 120)
[failure] 238-238: Ruff (E501)
myo/core.py:238:121: E501 Line too long (134 > 120)
myo/commands.py
[failure] 86-86: Ruff (E501)
myo/commands.py:86:121: E501 Line too long (138 > 120)
[failure] 127-127: Ruff (E501)
myo/commands.py:127:121: E501 Line too long (127 > 120)
[failure] 195-195: Ruff (E501)
myo/commands.py:195:121: E501 Line too long (127 > 120)
🔇 Additional comments (10)
myo/commands.py (3)
19-53: LGTM!The type hints and docstrings for the Command base class are clear, accurate, and improve code maintainability.
197-203: Critical bug fix verified.The high-byte extraction now correctly uses
>> 8instead of the previous>> 0xFF(which would have shifted by 255 bits). This properly encodes the 16-bit duration as two bytes in the payload.
138-156: LGTM!The input validation for LED RGB data is a good defensive addition that will catch malformed inputs early with a clear error message.
myo/types.py (3)
158-180: Critical bug fix verified.The
to_dict()method now correctly maps"unlock_pose"toself._unlock_poseinstead of incorrectly returningself._has_custom_classifier. This aligns with the__init__logic and ensures the serialized firmware info contains the correct unlock pose name.
359-401: LGTM!The
AggregatedDataclass is well-structured with clear type hints and comprehensive docstrings. The move fromcore.pytotypes.pyimproves code organization.
404-443: LGTM!The
EMGDataSingleclass provides a clean wrapper for single EMG samples with proper type hints and docstrings. Moving it totypes.pyimproves module organization.myo/core.py (4)
179-197: LGTM!The LED validation is comprehensive, checking length, type, and value range (0-255) with clear error messages. This defensive approach prevents invalid data from reaching the device.
705-746: LGTM!The helper methods
_get_emg_handles,_get_imu_handles, and_get_classifier_handlescleanly encapsulate the mode-to-handle mapping logic, reducing duplication in the start/stop methods.
365-367: LGTM!The consistent state validation across methods (
_client is Nonechecks, mode validation) with clear error messages significantly improves the API's safety and user experience.Also applies to: 394-396, 428-430, 462-464, 621-622, 640-642, 660-662, 757-760, 790-792, 803-806, 838-840, 852-854, 866-868, 880-882, 895-897
287-560: LGTM!Converting
MyoClientto an ABC with abstract handler methods enforces a clear contract for subclasses and improves type safety. The type hints and docstrings for each abstract method are accurate and comprehensive.
1 participant
Refactor: Code Organization and Type Hints
Overview
This PR refactors the codebase to improve code organization, add comprehensive type hints, fix bugs, and enhance maintainability.
Changes
1. Code Organization
AggregatedDataandEMGDataSinglefromcore.pytotypes.pyfor better organizationmyo/utils.pyand movedgatt_char_to_dictfunction there__init__.pyto import from correct modules2. Bug Fixes
FirmwareInfo.to_dict(): Correctedunlock_posefield to useself._unlock_poseinstead ofself._has_custom_classifierVibrate2command: Corrected byte shifting from>> 0xFFto>> 8for proper high byte extraction3. Type Hints and Documentation
core.py,commands.py,utils.py, andtypes.pyUniontype for Python 3.8+ compatibility4. Code Quality Improvements
_get_emg_handles(),_get_imu_handles(), and_get_classifier_handles()inMyoClient%formatting in logger callsMyoClient: Better state validation, improved notification handling, and more robust connection management5. Code Structure
Myo), client management (MyoClient), utilities (utils), and types (types)Files Modified
myo/types.py- AddedAggregatedDataandEMGDataSingleclasses, fixedFirmwareInfobugmyo/core.py- Refactored with type hints, improved error handling, extracted helper methodsmyo/commands.py- Added type hints and improved documentationmyo/utils.py- New utility module for GATT helper functionsmyo/__init__.py- Updated imports to reflect new structureTesting
Breaking Changes
None - this refactoring maintains full backward compatibility.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests
Chores