Improve code quality #327
Improve code quality #327
Conversation
WalkthroughTwo sets of changes: tests in Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Main Controller
participant SED as SED Node
participant Task as Awake Timer Task
Controller->>SED: request awake / schedule maintenance
note right of SED #DFF2E1: create_task(async _awake_timer)
SED->>Task: create_task(_awake_timer)
Task->>Task: await asyncio.sleep(timeout)
alt cancelled before timeout
Controller->>Task: cancel()
Task-->>SED: task cancelled -> no timeout action
SED-->>Controller: remains available / cancel ack
else sleep expires
Task-->>SED: timeout expired
note right of SED #FFF3D9: mark unavailable and log warning
SED-->>Controller: _available_update_state(False)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ 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)
✨ 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 (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 80.55% 80.47% -0.09%
==========================================
Files 36 36
Lines 7571 7559 -12
==========================================
- Hits 6099 6083 -16
- Misses 1472 1476 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (5)
tests/test_usb.py (5)
8-8: Optional: Prefer pytest.approx over math.isclose in testspytest.approx gives clearer diffs on failure, works well with scalars and sequences, and avoids an extra import. If you adopt the approx changes suggested below, you can drop the math import.
Apply if you migrate all isclose usages:
-import math
468-469: Nit: Also assert on the error message for stronger intentMatching a stable substring makes the test more explicit about why it failed.
Apply:
- with pytest.raises(pw_exceptions.StickError): + with pytest.raises(pw_exceptions.StickError, match="No mac address available"): stick.mac_stick
935-937: Optional: Use pytest.approx for numeric assertionsapprox improves readability and failure output. Tolerances remain the same.
Apply:
- assert math.isclose( - pu.last_second, 21.2780505980402, rel_tol=1e-09, abs_tol=1e-09 - ) + assert pu.last_second == pytest.approx(21.2780505980402, rel=1e-9, abs=1e-9)
1398-1403: Optional: Use pytest.approx here as wellKeeps a uniform assertion style across the suite.
Apply:
- assert math.isclose( - energy_counter_init.energy, - 0.07204743061527973, - rel_tol=1e-09, - abs_tol=1e-09, - ) + assert energy_counter_init.energy == pytest.approx( + 0.07204743061527973, rel=1e-9, abs=1e-9 + )
1412-1417: Optional: Align final energy assertion with pytest.approxFor consistency and better diffs.
Apply:
- assert math.isclose( - energy_counter_init.energy, - 0.08263379198066137, - rel_tol=1e-09, - abs_tol=1e-09, - ) + assert energy_counter_init.energy == pytest.approx( + 0.08263379198066137, rel=1e-9, abs=1e-9 + )
📜 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 (1)
tests/test_usb.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_usb.py (5)
plugwise_usb/__init__.py (2)
mac_stick(133-135)nodes(95-99)plugwise_usb/connection/__init__.py (1)
mac_stick(72-78)plugwise_usb/nodes/circle.py (4)
relay(197-199)relay_state(213-215)power(191-193)energy(170-172)plugwise_usb/nodes/node.py (6)
relay(290-294)relay_state(298-302)switch(314-318)power(272-276)energy(171-175)motion(241-245)plugwise_usb/nodes/helpers/counter.py (1)
energy(243-274)
🔇 Additional comments (2)
tests/test_usb.py (2)
855-860: LGTM: Consistently fixed raises pattern for unsupported featuresSame pattern applied here—looks good and consistent.
615-626: I wasn’t able to find any@property-decorated getters or a dynamic attribute handler forrelay,relay_state,switch,power,sense, orenergyin the codebase. Please manually verify that each of these members is implemented as a Python@property(or via__getattr__) so that accessing them indeed invokes the getter and raisesFeatureErroras intended.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/sed.py (1)
420-447: Maintenance interval detection is neutered by early overwrite of last_awakeYou overwrite self._last_awake[response.awake_type] (Line 429) before calling _detect_maintenance_interval (Lines 446-447). As a result, inside _detect_maintenance_interval the “previous” MAINTENANCE timestamp equals the current timestamp, causing the early-return on Line 473 and preventing detection from ever running on the second maintenance wake. This keeps maintenance_interval at the default and can lead to incorrect offline marking and scheduling.
Minimal fix: move the last-awake assignment to after the detection/branch logic, just before gather(). Example patch:
@@ - # Pre populate the last awake timestamp - if self._last_awake.get(response.awake_type) is None: - self._last_awake[response.awake_type] = response.timestamp + # Pre-populate the last awake timestamp if this is the first time we see this type + if self._last_awake.get(response.awake_type) is None: + self._last_awake[response.awake_type] = response.timestamp @@ - self._last_awake[response.awake_type] = response.timestamp @@ if response.awake_type == NodeAwakeResponseType.MAINTENANCE: self._last_awake_reason = "Maintenance" self._set_cache(CACHE_SED_AWAKE_REASON, "Maintenance") if not self._maintenance_interval_restored_from_cache: self._detect_maintenance_interval(response.timestamp) if self._ping_at_awake: tasks.append(self.update_ping_at_awake()) @@ - await gather(*tasks) + # Store the current timestamp after interval detection to preserve the previous one during detection + self._last_awake[response.awake_type] = response.timestamp + await gather(*tasks)Alternative (if you prefer explicitness): change _detect_maintenance_interval to accept previous and current timestamps, and pass the prior value captured before overwriting. I can draft that refactor if you prefer.
🧹 Nitpick comments (3)
plugwise_usb/nodes/sed.py (3)
119-121: Gracefully await timer cancellation on unloadYou cancel _awake_timer_task but don’t await it. In teardown paths this can occasionally leave a pending task until the loop cycles, which risks “Task was destroyed but it is pending!” during shutdowns. Suggest awaiting the cancellation and nulling the reference.
Apply within this method:
- if self._awake_timer_task is not None and not self._awake_timer_task.done(): - self._awake_timer_task.cancel() + if self._awake_timer_task is not None: + if not self._awake_timer_task.done(): + self._awake_timer_task.cancel() + try: + await self._awake_timer_task + except CancelledError: + pass + self._awake_timer_task = NoneAnd add the import (outside this hunk):
from asyncio import CancelledError
439-441: Avoid stacking duplicate _run_awake_tasks when multiple awake messages arrive quicklyIf several awake messages arrive in quick succession, you’ll schedule multiple _run_awake_tasks concurrently. While likely harmless, it’s wasted work. Consider canceling any still-running self._delayed_task before scheduling a new one.
- self._delayed_task = create_task( + if self._delayed_task is not None and not self._delayed_task.done(): + self._delayed_task.cancel() + self._delayed_task = create_task( self._run_awake_tasks(), name=f"Delayed update for {self._mac_in_str}" )
525-539: Make the timeout factor self-documenting and clamp to int seconds2.1 is a “2 intervals + 10% slack” heuristic. Encode that intent in variables and log an integer number of seconds to reduce noise. Also, consider logging at info level after the first occurrence to avoid warning fatigue.
- timeout_interval = self.maintenance_interval * 60 * 2.1 - await sleep(timeout_interval) + missed_cycles = 2 + jitter = 0.1 # allow 10% slack + timeout_secs = int(self.maintenance_interval * 60 * (missed_cycles + jitter)) + await sleep(timeout_secs) @@ - _LOGGER.warning( + _LOGGER.warning( "No awake message received from %s | last_maintenance_awake=%s | interval=%s (%s) | Marking node as unavailable", self.name, last_awake, self.maintenance_interval, - timeout_interval, + timeout_secs, )Optional: downgrade to info after the first warning per device; happy to sketch a small per-device backoff.
📜 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 (1)
plugwise_usb/nodes/sed.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/sed.py (2)
plugwise_usb/nodes/node.py (2)
name(218-222)_available_update_state(422-450)plugwise_usb/messages/responses.py (1)
NodeAwakeResponseType(87-95)
⏰ 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: Process test coverage
🔇 Additional comments (1)
plugwise_usb/nodes/sed.py (1)
515-520: Reset logic LGTMCanceling the previous timer task on each awake and spinning a fresh one via create_task keeps the timer consistent with the latest maintenance interval.
|
test improvements recommended by SonarCloud
Summary by CodeRabbit
Refactor
Tests
Chores