Catch error from #312 #321
Conversation
WalkthroughAdded an Ongoing CHANGELOG bullet for PR 321. In Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (2)
CHANGELOG.md (1)
5-5: Replace placeholders with actual PR and Issue linksThe CHANGELOG entry has empty links. Please fill in the PR number and the correct Issue URL to keep the log useful.
Proposed change:
-- PR [](): Catch error from Issue [#312]() +- PR [321](https://github.com/plugwise/python-plugwise-usb/pull/321): Catch error from Issue [#312](<INSERT_CORRECT_ISSUE_URL>)Can you confirm the correct repository for Issue #312 (plugwise/plugwise-beta vs plugwise/plugwise_usb-beta) so we can insert the proper URL?
plugwise_usb/helpers/util.py (1)
17-21: Good guard for None/non-strings; consider simplifying and keeping semantics explicitCatching TypeError here solves the reported issue when mac is None. To improve readability and avoid using exceptions for control flow, an explicit type check is cleaner while preserving the existing behavior (uppercase hex only).
Suggested refactor:
- try: - if not re.match("^[A-F0-9]+$", mac): - return False - except TypeError: - return False + if not isinstance(mac, str): + return False + if not re.match("^[A-F0-9]+$", mac): + return FalseOptional follow-up (if acceptable): use
re.fullmatchfor clarity instead of anchoring with ^$:- if not re.match("^[A-F0-9]+$", mac): + if re.fullmatch(r"[A-F0-9]+", mac) is None: return FalseIf helpful, I can add a small unit test asserting validate_mac(None) is False to prevent regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)plugwise_usb/helpers/util.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/helpers/util.py (1)
plugwise_usb/exceptions.py (1)
NodeError(24-25)
🪛 GitHub Actions: Latest commit
plugwise_usb/helpers/util.py
[error] 10-10: Ruff: F401 unused import 'NodeError' in plugwise_usb/helpers/util.py:10. Remove unused import. 1 fixable with --fix.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
==========================================
+ Coverage 80.50% 80.56% +0.06%
==========================================
Files 36 36
Lines 7555 7558 +3
==========================================
+ Hits 6082 6089 +7
+ Misses 1473 1469 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugwise_usb/helpers/util.py (3)
14-15: Align the type hint/docstring with the new behavior (accepting None).Since validate_mac now explicitly handles non-string inputs (e.g., None) by returning False, reflect that in the type hint and docstring for clarity and to help static analysis.
Apply this diff:
-def validate_mac(mac: str) -> bool: - """Validate the supplied string is in a MAC address format.""" +def validate_mac(mac: str | None) -> bool: + """Validate MAC-like hex string without separators; returns False for None/non-str."""
17-17: Prefer re.fullmatch over re.match with anchors for clarity.Functionally equivalent here, but fullmatch makes the intent explicit and avoids ^/$ anchors.
Apply this diff:
- if not re.match("^[A-F0-9]+$", mac): + if not re.fullmatch(r"[A-F0-9]+", mac):
22-25: Defensive: also catch TypeError on int(.., 16) conversion.Not strictly necessary given the earlier regex guard, but broadening the exception here makes this block robust if the first guard is ever refactored.
Apply this diff:
- except ValueError: + except (ValueError, TypeError): return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)plugwise_usb/helpers/util.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (1)
plugwise_usb/helpers/util.py (1)
16-20: Graceful handling of non-string MACs (fix for #312) — LGTMCatching TypeError around the regex check cleanly prevents crashes on None/bytes inputs and correctly returns False. This is a minimal, safe change with no API breakage.
Summary by CodeRabbit
Bug Fixes
Chores