fix: use timezone-aware datetime functions and update scheduler event names #343
Conversation
migrate bus api client to ovos-bus-client package
WalkthroughAdds deprecation for EventSchedulerInterface.init, replaces naive datetime usage with timezone-aware helpers (now_local, get_config_tz) in scheduling, updates now_utc construction, shifts scheduler update event name, adjusts unit test expectations, and updates CI workflow Python matrix and actions versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Scheduler as EventSchedulerInterface
participant Config as get_config_tz()
participant Broker as MessageBus
rect rgb(230,240,255)
Note right of Scheduler: Initialization (deprecated)
Caller->>Scheduler: instantiate()
Scheduler->>Caller: DeprecationWarning (emitted)
end
rect rgb(240,255,230)
Note right of Scheduler: Scheduling flow (new)
Caller->>Scheduler: schedule_event(when)
Scheduler->>Scheduler: now = now_local()
alt when is naive
Scheduler->>Config: get_config_tz()
Config-->>Scheduler: tz
Scheduler->>Scheduler: assign tz to when
end
Scheduler->>Broker: emit "mycroft.scheduler.update_event" / "mycroft.scheduler.scheduled_event"
Broker-->>Caller: event published
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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 (1)
ovos_utils/events.py (1)
211-217: Consider removing redundant deprecation warnings.Both the
@deprecateddecorator (line 211) and the manualwarnings.warn(lines 213-217) emit deprecation notices. The decorator already logs the deprecation vialog_deprecation, making the manual warning redundant.Choose one approach:
Option 1: Keep only the decorator
- @deprecated("EventSchedulerInterface moved to ovos_bus_client. 'from ovos_bus_client.apis.events import EventSchedulerInterface'", "1.0.0") def __init__(self, bus=None, skill_id=None): - warnings.warn( - "EventSchedulerInterface moved to ovos_bus_client. 'from ovos_bus_client.apis.events import EventSchedulerInterface'", - DeprecationWarning, - stacklevel=2, - )Option 2: Keep only the manual warning
- @deprecated("EventSchedulerInterface moved to ovos_bus_client. 'from ovos_bus_client.apis.events import EventSchedulerInterface'", "1.0.0") def __init__(self, bus=None, skill_id=None): warnings.warn( "EventSchedulerInterface moved to ovos_bus_client. 'from ovos_bus_client.apis.events import EventSchedulerInterface'", DeprecationWarning, stacklevel=2, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_utils/events.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_utils/events.py (2)
ovos_utils/log.py (1)
deprecated(306-324)ovos_utils/time.py (2)
now_local(33-43)get_config_tz(11-21)
🪛 Ruff (0.14.3)
ovos_utils/events.py
279-279: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
ovos_utils/events.py (3)
2-2: LGTM! Imports support timezone-aware datetime handling.The new imports are properly used for deprecation warnings and timezone-aware datetime operations.
Also applies to: 9-10
273-284: Excellent timezone handling fixes!The changes correctly address timezone issues:
- Line 277:
now_local()ensures timezone-aware datetime using the configured timezone- Lines 280-284: Properly handles naive datetime by assigning the configured timezone using
replace(tzinfo=...), which is correct - it treats the naive datetime as already being in the target timezone rather than converting itThe implementation and comments are clear and accurate.
351-351: LGTM! Consistent timezone-aware datetime usage.The change from
datetime.now()tonow_local()ensures the initial scheduling time for repeating events uses the configured timezone, consistent with the fix at line 277.
caught by coderabbit, been broken since mycroft times...
why do we test if we are just testing the bad behaviour...
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unittests/test_events.py (1)
266-272: TODO: Rewrite the timezone-naive test case.The commented-out test for scheduling TZ-naive events should be updated to reflect the new behavior where naive datetimes are assigned the configured timezone from mycroft.conf.
Would you like me to help generate an updated test case that validates the new timezone handling behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/unit_tests.yml(2 hunks)ovos_utils/events.py(6 hunks)ovos_utils/time.py(1 hunks)test/unittests/test_events.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/unittests/test_events.py (1)
ovos_utils/events.py (1)
_schedule_event(255-308)
ovos_utils/events.py (3)
ovos_utils/fakebus.py (4)
FakeMessage(179-329)Message(332-343)dig_for_message(9-15)forward(242-257)ovos_utils/log.py (1)
deprecated(306-324)ovos_utils/time.py (2)
now_local(33-43)get_config_tz(11-21)
🪛 Ruff (0.14.3)
ovos_utils/events.py
279-279: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: unit_tests (3.12)
🔇 Additional comments (7)
.github/workflows/unit_tests.yml (1)
40-64: LGTM! CI updates are appropriate.The Python version matrix update (dropping 3.9, adding 3.11-3.12) and GitHub Actions version upgrades (checkout@v4, setup-python@v5) are solid maintenance improvements.
ovos_utils/time.py (1)
30-30: LGTM! Modern timezone-aware datetime construction.Replacing
datetime.utcnow().replace(tzinfo=...)withdatetime.now(tz=gettz("UTC"))is the recommended approach for creating timezone-aware UTC datetimes, especially with Python 3.12+ whereutcnow()is deprecated.test/unittests/test_events.py (1)
279-279: Verify the expected message count aligns with test execution.The assertion has been updated from 3 to 2 messages, reflecting the removal of the TZ-naive test path. Confirm this matches the actual number of scheduled events in the test.
ovos_utils/events.py (4)
277-277: LGTM! Consistent timezone-aware time handling.Using
now_local()instead ofdatetime.now()ensures the calculated event time is timezone-aware and consistent with the configured timezone.
280-284: LGTM! Proper timezone handling for naive datetimes.Assigning
get_config_tz()to naive datetime objects before timestamp conversion ensures consistency with mycroft.conf timezone settings, avoiding platform-specific Cmktime()behavior.
351-351: LGTM! Consistent timezone-aware scheduling.Using
now_local()ensures the initial repeating event trigger time is timezone-aware.
370-370: No issues found. The event name change is correct and consistent.The verification confirms:
- The new event name
mycroft.scheduler.update_eventis in place (line 370)- The old event name
mycroft.schedule.update_eventdoes not exist in the codebase—no internal code needs updating- The naming is consistent with related scheduler events:
mycroft.scheduler.schedule_eventandmycroft.scheduler.remove_eventThe change is an intentional fix for naming consistency and does not break any internal consumers.
JarbasAl
changed the title
migrate bus api client to ovos-bus-client package
Summary by CodeRabbit
Deprecations
Bug Fixes
Tests
Chores