Catch serial.tools.list_ports import exception for unsupported platforms - #970
Catch serial.tools.list_ports import exception for unsupported platforms#970sqbi-q wants to merge 1 commit into
Conversation
|
|
1 similar comment
|
|
📝 WalkthroughWalkthrough
ChangesSerial port import handling
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meshtastic/util.py`:
- Around line 22-27: Initialize list_ports to None in the ImportError handler,
then update findPorts() to return an empty list before calling
list_ports.comports() when the fallback is unavailable. Preserve normal port
discovery when the import succeeds, and add a regression test covering the
failed-import path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 259d213f-e2ae-44d0-8fef-3c2de5187898
📒 Files selected for processing (1)
meshtastic/util.py
| try: | ||
| import serial.tools.list_ports # type: ignore[import-untyped] | ||
| except ImportError as e: | ||
| print("Error: ", e) | ||
| print("Error: Cannot list ports on this platform") | ||
| print("Warning: Skipping import serial.tools.list_ports") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Define a fallback for failed imports.
When this import raises ImportError, the handler does not initialize a usable fallback. However, findPorts() still calls serial.tools.list_ports.comports() at Line 162. On an unsupported platform, findPorts() can raise NameError or AttributeError instead of returning no ports.
Store a sentinel such as list_ports = None, check it at the start of findPorts(), and return an empty list before accessing the unavailable module. Add a regression test for the failed-import path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshtastic/util.py` around lines 22 - 27, Initialize list_ports to None in
the ImportError handler, then update findPorts() to return an empty list before
calling list_ports.comports() when the fallback is unavailable. Preserve normal
port discovery when the import succeeds, and add a regression test covering the
failed-import path.
My fix for #969.
Summary by CodeRabbit