-
Notifications
You must be signed in to change notification settings - Fork 2
Solve monthly overflow as reported in #399 #400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughReplaces "Ongoing" with a v0.47.2 changelog entry, refactors Circle.clock_synchronize() to use an extracted _send_clock_set_req() and early weekday drift check, adds a month-end clock-rollover test, extends test response data, and bumps pyproject version to 0.47.2. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
==========================================
+ Coverage 82.05% 82.08% +0.03%
==========================================
Files 36 36
Lines 8156 8181 +25
==========================================
+ Hits 6692 6715 +23
- Misses 1464 1466 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugwise_usb/nodes/circle.py`:
- Around line 885-891: The month overflow handling around calculating
circle_timestamp is wrong: when (difference := days_diff - days_to_end_of_month)
> 0 you adjust corrected_day but do not advance the month, producing incorrect
dates; replace the manual day correction logic (the block using
last_day_of_month, days_to_end_of_month, corrected_day and
dt_now.replace(day=...)) by computing circle_timestamp as dt_now +
timedelta(days=days_diff) (using the existing dt_now and days_diff variables) so
month/year boundaries are handled correctly and remove the fragile
corrected_day/replace logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes clock synchronization date calculation when the target weekday crosses a month boundary (avoiding invalid datetime.replace(day=...) behavior at month-end).
Changes:
- Update
clock_synchronize()to compute the target date viadt_now + timedelta(days=...)to handle month rollovers safely. - Add an “Ongoing” changelog entry referencing the PR/issue.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
plugwise_usb/nodes/circle.py |
Switches clock-sync date computation to timedelta arithmetic to prevent month-end overflow. |
CHANGELOG.md |
Documents the fix in the ongoing changelog section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_usb.py`:
- Around line 3043-3083: The test_test_clock_synchronize_month_overflow case
doesn't force a month rollover because the frozen date "2026-01-31" is already
Saturday (weekday 5) and mock_clock_get_send sets response.day_of_week.value = 5
so days_diff == 0; update the test so the mocked day_of_week differs by +1 to
force crossing into February (e.g., set response.day_of_week.value = 6) or
change the frozen time in the `@freeze_time` decorator to a Friday date so that
mock_clock_get_send's day_of_week=5 results in a +1 day rollover; modify the
mock_clock_get_send implementation (and/or the freeze_time value) inside
test_clock_synchronize_month_overflow to ensure days_diff != 0 and the code path
that adds timedelta(days=1) is exercised.
dirixmjm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Bedankt voor de actie.
|
@dirixmjm Thanks, but I'm not there yet, trying to recreate the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugwise_usb/nodes/circle.py`:
- Around line 912-913: The docstring for async def _send_clock_set_req currently
incorrectly references "CirclePlusRealTimeClockSetRequest"; update it to
accurately describe that this method sends a CircleClockSetRequest (e.g., "Send
CircleClockSetRequest.") so the docstring matches the implemented behavior in
_send_clock_set_req and avoids confusion with CirclePlus methods.
- Around line 883-888: The early return calls _send_clock_set_req() when
dt_now.weekday() != response.day_of_week.value but the null-check for
self._node_protocols happens later, which can cause an AttributeError; either
move the existing self._node_protocols is None check so it runs before the early
return or add the same guard at the start of _send_clock_set_req() (and
return/raise gracefully) so _send_clock_set_req() never accesses
self._node_protocols.max when _node_protocols is None; update the call site or
the helper accordingly and ensure logging/behavior remains consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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/circle.py (1)
910-923: Helper extraction looks good, but needs the null guard.The refactoring to extract
_send_clock_set_req()follows the pattern established incircle_plus.pyand improves code reuse. The implementation is correct, but as noted above, the method should include the_node_protocolsnull check internally to protect all callers.Once the null guard is added to the helper, the redundant check at lines 906-909 can be removed since both call sites will be protected by the method itself.
|




Summary by CodeRabbit
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.