Add little surprise #887
Add little surprise #887
Conversation
WalkthroughThe changes introduce a new scheduling mechanism within the Changes
Sequence Diagram(s)sequenceDiagram
participant Program
participant _SpecialEventScheduler
Program->>_SpecialEventScheduler: Initialize
Program->>_SpecialEventScheduler: Setup Schedulers
_SpecialEventScheduler->>Program: Return Scheduler
Program->>_SpecialEventScheduler: Schedule Event
_SpecialEventScheduler->>Program: Event Scheduled
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Outside diff range and nitpick comments (10)
src/program/_hidden_scheduler.py (3)
1-7: Consider removing commented test code and unused importThe
timedeltaimport is only used in the commented testing code. If the testing code is no longer needed, both the import and the comment could be removed for cleaner code.-from datetime import datetime, timedelta +from datetime import datetime import sched import threading import time from program.media.item import MediaItem🧰 Tools
🪛 Ruff
1-1:
datetime.timedeltaimported but unusedRemove unused import:
datetime.timedelta(F401)
19-27: Consider timezone handling and edge casesThe date calculation could be improved by:
- Adding timezone awareness to handle different server locations
- Using more efficient datetime operations
- Adding validation for system time changes
- def _get_next_april_first(self): + def _get_next_april_first(self) -> datetime: """Get next April 1st at midnight.""" - today = datetime.now() + today = datetime.now().replace(hour=0, minute=0, second=0, microsecond=0) - current_year = today.year - april_first = datetime(current_year, 4, 1, 0, 0, 0) + april_first = today.replace(month=4, day=1) if today > april_first: - april_first = datetime(current_year + 1, 4, 1, 0, 0, 0) + april_first = april_first.replace(year=today.year + 1) return april_first
1-33: Consider adding configuration options and loggingWhile this April Fools' feature is creative, consider these architectural improvements:
- Add a configuration flag to enable/disable the feature
- Make the IMDB ID configurable
- Add proper logging for monitoring and debugging
- Consider adding this feature to the documentation with a spoiler warning
This would make the feature more maintainable and controllable in production environments.
Would you like me to provide an example implementation with these improvements?
🧰 Tools
🪛 Ruff
1-1:
datetime.timedeltaimported but unusedRemove unused import:
datetime.timedelta(F401)
src/program/program.py (7)
Line range hint
485-577: Improve exception handling in_init_db_from_symlinksThe current exception handling in the
_init_db_from_symlinksmethod may cause the entire initialization process to abort if an exception occurs in one chunk. To enhance robustness:
Handle Exceptions Without Halting Execution
Instead of re-raising exceptions after logging, consider handling them and allowing the process to continue with the next chunk or item. This prevents a single failure from stopping the entire initialization.
Log Errors Immediately
Logging errors as they occur provides immediate feedback and simplifies error tracking. Accumulating errors in a list may delay visibility into issues.
Ensure Proper Rollback
When an exception occurs, ensure that the session is properly rolled back to maintain database integrity.
Apply the following changes to improve exception handling:
@@ -548,7 +548,9 @@ logger.exception(f"Error processing {item.log_string}: {e}") - raise # Re-raise to trigger rollback + session.rollback() + errors.append(f"Exception occurred while processing {item.log_string}: {e}") + continue # Continue with the next item @@ -557,7 +559,9 @@ session.rollback() logger.error(f"Failed to process chunk {i//chunk_size + 1}, rolling back all changes: {str(e)}") - raise # Re-raise to abort the entire process + errors.append(f"Failed to process chunk {i//chunk_size + 1}: {e}") + continue # Continue with the next chunkBy adjusting the exception handling, the initialization process becomes more resilient, allowing it to continue despite encountering errors with certain items or chunks.
Line range hint
527-529: PreventAttributeErrorwhen checkingimdb_idBefore accessing
item.imdb_id, ensure thatitemis notNoneand that it has theimdb_idattribute to prevent potentialAttributeError.Apply this fix to safely check
imdb_id:-if not item or item.imdb_id in added_items: +if not item or not hasattr(item, 'imdb_id') or item.imdb_id in added_items:
Line range hint
531-533: Optimize database queries to check for existing itemsUsing
db_functions.get_item_by_idmay not be the most efficient method for checking the existence of an item within the current session. Consider using the session's query capabilities to reduce overhead.Refactor the code as follows:
-if db_functions.get_item_by_id(item.id, session=session): +if session.query(MediaItem).filter_by(id=item.id).first():This change leverages the current session for a more efficient database lookup.
Line range hint
564-566: Simplify error logging during initializationAccumulating errors in a list and then iterating over them to log may be unnecessary. Instead, log errors immediately when they occur. This approach provides real-time feedback and reduces code complexity.
Modify the error handling as follows:
-logger.error("Errors encountered during initialization") -for error in errors: - logger.error(error) +# Errors are now logged immediately when they occur; no need to iterate over them later.
Line range hint
554-554: Commit after each successful chunk processingCurrently, the session commits only if the entire chunk is processed without errors. To avoid losing progress from successfully processed items within a chunk when an error occurs, consider committing after processing each item or handling exceptions at the item level.
Adjust the commit logic:
-# Only commit if the entire chunk was successful -session.commit() +# Commit after processing each item # Inside the item processing loop ... session.commit()This ensures that successfully enhanced items are saved even if a subsequent item causes an error.
Line range hint
548-549: Avoid raising exceptions after loggingAfter logging an exception with
logger.exception, re-raising it immediately may not be necessary and can halt processing. Since the exception details are already logged, consider handling it without re-raising.Modify the exception handling:
logger.exception(f"Error processing {item.log_string}: {e}") -raise # Re-raise to trigger rollback +session.rollback() +continue # Proceed with the next item
Line range hint
510-510: Ensure default value forSYMLINK_MAX_WORKERSis an integerWhen retrieving
SYMLINK_MAX_WORKERSfrom the environment, ensure that the default value provided toint()is an integer to avoid aTypeError.Apply this fix:
-workers = int(os.getenv("SYMLINK_MAX_WORKERS", 4)) +workers = int(os.getenv("SYMLINK_MAX_WORKERS", "4"))By passing the default value as a string, we accommodate situations where environment variables are strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/program/_hidden_scheduler.py(1 hunks)src/program/program.py(2 hunks)
🧰 Additional context used
🪛 Ruff
src/program/_hidden_scheduler.py
1-1: datetime.timedelta imported but unused
Remove unused import: datetime.timedelta
(F401)
🔇 Additional comments (4)
src/program/_hidden_scheduler.py (1)
28-33:
Add error handling and document the surprise content
The item addition needs improvements:
- Error handling for MediaItem operations
- Documentation about the selected content
- Validation of program state
def _add_items(self):
"""Add special items to queue."""
+ # IMDB ID: tt0090655 - Police Academy 3: Back in Training (1986)
+ # Selected as a lighthearted comedy for April Fools' Day
+ try:
+ if not hasattr(self.program, 'em'):
+ raise AttributeError("Program missing media item manager")
item = MediaItem(
{"imdb_id": "tt0090655", "requested_by": "system", "requested_at": datetime.now()}
)
- self.program.em.add_item(item)
+ self.program.em.add_item(item)
+ except Exception as e:
+ # Log error but don't raise to avoid daemon thread termination
+ print(f"Failed to add April Fools' movie: {e}")Also, please verify if this movie choice is appropriate for your audience.
src/program/program.py (3)
34-34: Importing _HiddenScheduler to enhance modularity
The addition of _HiddenScheduler improves the modularity of the scheduling functionalities within the Program class. This separation of concerns can aid in maintainability and clarity.
68-68: Initializing _scheduler in the constructor
Initializing self._scheduler = _HiddenScheduler(self) within the __init__ method ensures that the scheduler is set up as soon as a Program instance is created. Verify that this early initialization does not introduce any unintended side effects or performance overhead during startup.
Line range hint 495-497: Handle potential exceptions when running SymlinkLibrary
The call to self.services[SymlinkLibrary].run() may raise exceptions that are not currently caught, which could halt the initialization process.
Consider wrapping this call in a try-except block to handle potential exceptions gracefully.
src/program/_hidden_scheduler.py
Outdated
| class _HiddenScheduler: | ||
| def __init__(self, program): | ||
| self.program = program | ||
| scheduler = sched.scheduler(time.time, time.sleep) | ||
| #next_time = datetime.now() + timedelta(minutes=1) # For testing | ||
| next_time = self._get_next_april_first() # For production | ||
| scheduler.enterabs(next_time.timestamp(), 1, self._add_items) | ||
| scheduler_thread = threading.Thread(target=scheduler.run, daemon=True) | ||
| scheduler_thread.start() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and error handling
The initialization lacks type hints and error handling. Consider adding:
- Type hints for better code maintainability
- Error handling for scheduler operations
- A method to cleanly stop the scheduler
class _HiddenScheduler:
- def __init__(self, program):
+ def __init__(self, program: 'Program') -> None:
self.program = program
+ self._scheduler = None
+ try:
- scheduler = sched.scheduler(time.time, time.sleep)
+ self._scheduler = sched.scheduler(time.time, time.sleep)
- next_time = self._get_next_april_first() # For production
- scheduler.enterabs(next_time.timestamp(), 1, self._add_items)
- scheduler_thread = threading.Thread(target=scheduler.run, daemon=True)
- scheduler_thread.start()
+ next_time = self._get_next_april_first()
+ self._scheduler.enterabs(next_time.timestamp(), 1, self._add_items)
+ self._thread = threading.Thread(target=self._scheduler.run, daemon=True)
+ self._thread.start()
+ except Exception as e:
+ raise RuntimeError(f"Failed to initialize scheduler: {e}")
+ def stop(self) -> None:
+ """Stop the scheduler and clean up resources."""
+ if self._scheduler:
+ for event in self._scheduler.queue:
+ self._scheduler.cancel(event)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class _HiddenScheduler: | |
| def __init__(self, program): | |
| self.program = program | |
| scheduler = sched.scheduler(time.time, time.sleep) | |
| #next_time = datetime.now() + timedelta(minutes=1) # For testing | |
| next_time = self._get_next_april_first() # For production | |
| scheduler.enterabs(next_time.timestamp(), 1, self._add_items) | |
| scheduler_thread = threading.Thread(target=scheduler.run, daemon=True) | |
| scheduler_thread.start() | |
| class _HiddenScheduler: | |
| def __init__(self, program: 'Program') -> None: | |
| self.program = program | |
| self._scheduler = None | |
| try: | |
| self._scheduler = sched.scheduler(time.time, time.sleep) | |
| next_time = self._get_next_april_first() | |
| self._scheduler.enterabs(next_time.timestamp(), 1, self._add_items) | |
| self._thread = threading.Thread(target=self._scheduler.run, daemon=True) | |
| self._thread.start() | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to initialize scheduler: {e}") | |
| def stop(self) -> None: | |
| """Stop the scheduler and clean up resources.""" | |
| if self._scheduler: | |
| for event in self._scheduler.queue: | |
| self._scheduler.cancel(event) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/program/_special_event_scheduler.py (2)
30-30: Remove commented-out debug code for clean production codeThe
debug_minutesparameter in line 30 is commented out with a note to remove it in production. To maintain a clean codebase, it's better to remove unnecessary commented code.Apply this diff to remove the commented line:
- #debug_minutes=1, # Remove this in production
25-40: Consider using a list to manage events for scalabilityCurrently, events are stored in a dictionary with event names as keys. As the number of events grows, maintaining this dictionary might become cumbersome.
Consider using a list or other data structure to manage events without explicit keys:
- self.events: Dict[str, SpecialEvent] = { - "april_fools": SpecialEvent( + self.events: List[SpecialEvent] = [ + SpecialEvent( imdb_ids=["tt0090655"], # April Fool's Day month=4, day=1, log_event=False, ), - "christmas": SpecialEvent( + SpecialEvent( imdb_ids=[ "tt0099785", # Home Alone ], month=12, day=25, ), - } + ]This simplifies the event management and reduces dependency on event names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/program/_special_event_scheduler.py(1 hunks)src/program/program.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/program/program.py
🔇 Additional comments (1)
src/program/_special_event_scheduler.py (1)
83-89: Verify thread safety when adding items to the program
The _add_items method adds items to self.program.em from a separate thread. Ensure that the add_item method and any related data structures are thread-safe to prevent potential data races or inconsistent states.
Run the following script to check for thread safety in add_item methods:
This script searches for the add_item method definition and checks for the use of locks or other synchronization mechanisms in the codebase.
| def _add_items(self, event: SpecialEvent): | ||
| """Add special items to queue.""" | ||
| if event.log_event: | ||
| logger.debug("Adding system items for special event") | ||
| for imdb_id in event.imdb_ids: | ||
| item = MediaItem({ | ||
| "imdb_id": imdb_id, | ||
| "requested_by": event.requested_by, | ||
| "requested_at": datetime.now() | ||
| }) | ||
| self.program.em.add_item(item) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception handling to prevent silent thread failures
The _add_items method runs in a separate thread but lacks exception handling. If an exception occurs, the thread may terminate silently, making debugging difficult.
Consider adding a try-except block to catch and log exceptions:
def _add_items(self, event: SpecialEvent):
"""Add special items to queue."""
try:
if event.log_event:
logger.debug("Adding system items for special event")
for imdb_id in event.imdb_ids:
item = MediaItem({
"imdb_id": imdb_id,
"requested_by": event.requested_by,
"requested_at": datetime.now()
})
self.program.em.add_item(item)
except Exception as e:
logger.error(f"Error adding items for event {event}: {e}", exc_info=True)This ensures that any exceptions are logged, aiding in troubleshooting.
| def _setup_schedulers(self): | ||
| """Setup schedulers for all special events.""" | ||
| for event_name, event in self.events.items(): | ||
| scheduler = sched.scheduler(time.time, time.sleep) | ||
| next_time = ( | ||
| datetime.now() + timedelta(minutes=event.debug_minutes) | ||
| if event.debug_minutes is not None | ||
| else self._get_next_date(event.month, event.day) | ||
| ) | ||
| scheduler.enterabs( | ||
| next_time.timestamp(), | ||
| 1, | ||
| self._add_items, | ||
| argument=(event,) | ||
| ) | ||
| scheduler_thread = threading.Thread( | ||
| target=scheduler.run, | ||
| name=f"SpecialEvent_{event_name}", | ||
| daemon=True | ||
| ) | ||
| scheduler_thread.start() | ||
| if event.log_event: | ||
| logger.debug(f"Scheduled special event \"{event_name}\" for {next_time}") | ||
|
|
There was a problem hiding this comment.
Ensure events are rescheduled after execution for recurring events
Currently, each special event is scheduled to run only once. After the event is triggered, it is not rescheduled for future occurrences (e.g., the next year). This means that recurring events will not be automatically handled unless the program is restarted.
To ensure that events are rescheduled annually, consider modifying the _add_items method to reschedule the event after it has been executed. Here's how you can adjust the code:
def _add_items(self, event: SpecialEvent):
"""Add special items to queue."""
+ # Reschedule the event for the next occurrence
+ next_time = self._get_next_date(event.month, event.day)
+ scheduler = sched.scheduler(time.time, time.sleep)
+ scheduler.enterabs(
+ next_time.timestamp(),
+ 1,
+ self._add_items,
+ argument=(event,)
+ )
+ scheduler_thread = threading.Thread(
+ target=scheduler.run,
+ name=f"SpecialEvent_{event.month}_{event.day}",
+ daemon=True
+ )
+ scheduler_thread.start()
if event.log_event:
logger.debug("Adding system items for special event")
for imdb_id in event.imdb_ids:
item = MediaItem({
"imdb_id": imdb_id,
"requested_by": event.requested_by,
"requested_at": datetime.now()
})
self.program.em.add_item(item)This modification reschedules the event for its next occurrence, ensuring that recurring events continue to be handled without requiring a program restart.
Committable suggestion skipped: line range outside the PR's diff.
Pull Request Check List
Resolves: #issue-number-here
Description:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor