Fix --reset-hour parameter not being used in reset time calculations #96
Conversation
|
""" WalkthroughThe changes implement support for a customizable reset hour and timezone in session time calculations. They introduce new parameters to relevant methods, add logic to prioritize user-supplied reset hour and timezone, and provide a utility for calculating the next reset time. Comprehensive tests are added to ensure correct behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI/User
participant DisplayController
participant SessionCalculator
participant TimezoneHandler
CLI/User->>DisplayController: Provide reset_hour, timezone (via args)
DisplayController->>SessionCalculator: calculate_time_data(..., reset_hour, timezone)
SessionCalculator->>TimezoneHandler: get_next_reset_time(current_time, reset_hour, timezone)
TimezoneHandler-->>SessionCalculator: next_reset_time
SessionCalculator-->>DisplayController: time_data (using next_reset_time)
DisplayController-->>CLI/User: Display updated reset time
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (16)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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)
src/claude_monitor/utils/time_utils.py (1)
538-545: Consider usingnormalize()for more robust DST handling.While the current DST handling works, you could make it more robust by using pytz's
normalize()method after localization to handle DST transitions properly.except Exception as e: logger.warning(f"DST handling issue: {e}, using naive datetime") naive_dt = datetime.combine( next_reset_date, datetime.min.time().replace(hour=next_reset_hour) ) - next_reset = target_tz.localize(naive_dt) + next_reset = target_tz.normalize(target_tz.localize(naive_dt))src/tests/test_time_utils.py (1)
803-821: Add test case for seconds handling.Based on the edge case identified in the main implementation, consider adding a test to verify behavior when the current time has non-zero seconds at the reset hour.
Add this test after
test_custom_reset_hour_after_exact_time:def test_custom_reset_hour_with_seconds(self) -> None: """Test when current time is at reset hour but with non-zero seconds.""" handler = TimezoneHandler("Asia/Seoul") # Set current time to 1:00:30 PM in Asia/Seoul (30 seconds past reset hour) current_time = pytz.timezone("Asia/Seoul").localize( datetime(2024, 1, 1, 13, 0, 30) ) # Call get_next_reset_time with reset_hour=13 (1 PM) reset_time = handler.get_next_reset_time( current_time, reset_hour=13, timezone_str="Asia/Seoul" ) # Should return tomorrow's reset time since we're past the hour expected = pytz.timezone("Asia/Seoul").localize( datetime(2024, 1, 2, 13, 0, 0) ) assert reset_time == expected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/claude_monitor/ui/display_controller.py(4 hunks)src/claude_monitor/utils/time_utils.py(2 hunks)src/tests/test_time_utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/claude_monitor/utils/time_utils.py (2)
src/tests/test_calculations.py (1)
current_time(157-159)src/claude_monitor/error_handling.py (1)
report_error(21-53)
🔇 Additional comments (8)
src/claude_monitor/utils/time_utils.py (2)
460-481: LGTM! Well-documented method with clear purpose.The method signature and documentation are comprehensive, providing clear examples and explaining all parameters and return values.
552-564: Excellent error handling with appropriate fallback.The error handling properly reports the exception with context and falls back to the original behavior (current time + 5 hours), ensuring the application continues to function even if the new logic fails.
src/tests/test_time_utils.py (3)
762-783: Well-structured test for same-day reset scenario.The test properly validates that when the current time is before the reset hour, it returns the reset time on the same day. Good use of timezone-aware datetime objects.
928-943: Comprehensive error handling test.Excellent test that verifies the fallback behavior when an exception occurs. The use of mocking to simulate an error and verification of both the fallback result and error reporting is thorough.
945-977: Excellent DST and timezone combination testing.The test comprehensively validates the method across different timezones including during DST periods (June 15). The assertions properly verify that reset times are at the correct hour in the target timezone.
src/claude_monitor/ui/display_controller.py (3)
74-84: Clean parameter addition maintaining backward compatibility.The addition of optional
reset_hourandtimezoneparameters is well-implemented, preserving existing functionality while enabling the new feature.
351-356: Safe attribute access with proper defaults.Good use of
getattrwithNonedefaults to handle cases where the args object might not have these attributes, ensuring backward compatibility.
618-636: Correct implementation of reset time priority logic.The implementation properly prioritizes:
- User-specified custom reset hour (if provided)
- API-provided end time (backward compatibility)
- Default 5-hour intervals (fallback)
This matches the PR objectives perfectly.
The --reset-hour parameter was being stored but not actually used in reset time calculations. The monitor would always use session end times from the API or default 5-hour intervals, ignoring the user's custom reset hour setting. This fix: - Adds get_next_reset_time() method to TimezoneHandler class - Prioritizes user's custom reset hour over API end times - Maintains backward compatibility with existing behavior - Handles timezone conversions correctly - Includes comprehensive test coverage Fixes Maciek-roboblog#95
miraclebakelaser
force-pushed
the
fix/reset-hour-timezone-calculation
branch
from
2edda2f to
ce84971
Compare
|
@Maciek-roboblog lmk your thoughts. If you're cool with the PR I have another enhancement lined up that'll clear the reset hour from the params file if we're past the reset hour. |
1 participant
Description
Fixes the
--reset-hourparameter which was not being used in reset time calculations. The monitor was ignoring the user's custom reset hour setting and falling back to session end times from the API or default 5-hour intervals.Problem
When using
--reset-hour, the monitor would display an incorrect reset time based on the API session end time or default intervals, completely ignoring the user-specified reset hour.Solution
get_next_reset_time()method toTimezoneHandlerclass that calculates the next occurrence of a specific hour in a given timezone.DisplayControllerto prioritize user's custom reset hour over API end times.Testing
Root Cause
The refactored version of the application removed the
get_next_reset_time()function that was responsible for calculating the next occurrence of a specific hour in a given timezone. The current code only handled API-provided end times or fell back to a simple time offset, making the--reset-hourparameter effectively non-functional.Fixes #95
Summary by CodeRabbit
New Features
Bug Fixes
Tests