fix(logos): Add ncaam/ncaaw sport key aliases for basketball plugin #200
Conversation
📝 WalkthroughWalkthroughAdded NCAA basketball API endpoints for men's and women's college basketball (ncaam, ncaaw_basketball, ncaaw) with corresponding ESPN API URLs and expanded logo directory mappings to support these new NCAA basketball league keys without modifying existing logic. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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: 3
🤖 Fix all issues with AI agents
In `@docs/MULTI_ROOT_WORKSPACE_SETUP.md`:
- Around line 56-59: Replace the reference to the removed script
clone_plugin_repos.py with clear guidance: remove the call to python3
scripts/clone_plugin_repos.py and instead instruct readers to manually clone
required plugin repositories into the parent directory of LEDMatrix (or provide
their preferred clone commands), then run the existing setup script python3
scripts/setup_plugin_repos.py (refer to setup_plugin_repos.py) under the
LEDMatrix directory; update the section header (e.g., "Initial Setup") and the
example to show cd /home/chuck/Github/LEDMatrix followed by running
scripts/setup_plugin_repos.py so readers have a concrete, working alternative.
In `@scripts/setup_plugin_repos.py`:
- Around line 87-89: The except block that prints the error for plugin['name']
then calls link_path.unlink() can itself raise and abort processing; modify the
error-handling so unlink is performed safely—either call
link_path.unlink(missing_ok=True) if supported or wrap link_path.unlink() in its
own try/except that catches and logs any unlink-specific exception (referencing
link_path.unlink() and the surrounding except handling for plugin['name']) so
failures to remove the link do not propagate and stop processing other plugins.
In `@src/cache_manager.py`:
- Around line 587-593: The cleanup_disk_cache method currently declares a force:
bool parameter but never uses it; update cleanup_disk_cache to honor force by
adding a time-based skip that returns early when recent cleanup ran (e.g.,
consult self._last_cleanup_time or similar) unless force is True, and update the
docstring accordingly; if you prefer to remove the behavior instead, remove the
force parameter from the method signature and all callers (and adjust the
docstring) to eliminate the unused parameter. Ensure the change targets the
cleanup_disk_cache method and any references to self._last_cleanup_time or
callers that expect the force argument.
🧹 Nitpick comments (10)
docs/MULTI_ROOT_WORKSPACE_SETUP.md (2)
16-29: Add language specifier for markdown lint compliance.The code block is missing a language identifier.
Suggested fix
-``` +```text /home/chuck/Github/
17-18: Consider using generic paths for broader applicability.The documentation uses hardcoded paths like
/home/chuck/Github/throughout. Consider using a placeholder like~/Github/or<your-github-directory>/to make the documentation applicable to all developers.Also applies to: 35-35, 57-57, 64-65, 78-79, 114-114, 123-123, 130-131, 149-150, 159-159, 167-167, 174-174
scripts/setup_plugin_repos.py (4)
61-61: Remove extraneous f-string prefixes.These strings have no placeholders, so the
fprefix is unnecessary. Based on Ruff F541.Suggested fix
- print(f"Setting up plugin repository links...") + print("Setting up plugin repository links...")- print(f" Skipping (manual cleanup required)") + print(" Skipping (manual cleanup required)")Also applies to: 93-93
90-95: Non-symlink conflict counted as "skipped" rather than "error".When an existing directory/file blocks symlink creation, the script increments
skippedbut the message indicates "manual cleanup required." This is arguably an error condition that should incrementerrorsinstead, as the plugin won't be accessible.Suggested fix
else: # It's a directory/file, not a symlink print(f" ⚠️ {plugin['name']} - {link_path.name} exists but is not a symlink") - print(f" Skipping (manual cleanup required)") - skipped += 1 + print(" Skipping (manual cleanup required)") + errors += 1 continue
104-106: Consider catching specific exceptions.Catching
OSError(which includesPermissionError,FileExistsError) would be more precise than bareException. As per coding guidelines, catch specific exceptions, not bareexcept:statements.Suggested fix
- except Exception as e: + except OSError as e: print(f" ✗ {plugin['name']} - failed to create link: {e}") errors += 1
114-118: Placeholder function with no implementation.
update_config_path()is documented as an alternative approach but has no implementation. Consider removing it or adding a TODO comment to track if it should be implemented.src/cache/disk_cache.py (2)
301-340: Consider reducing lock contention for large cache directories.The lock is held for the entire directory scan and file deletion loop. On a Raspberry Pi with many cache files, this could block other cache operations (reads/writes) for an extended period.
Consider collecting the list of files to delete while holding the lock, then releasing it before performing deletions, or processing in batches.
♻️ Suggested approach
try: - with self._lock: - for filename in os.listdir(self.cache_dir): - if not filename.endswith('.json'): - continue - - stats['files_scanned'] += 1 - file_path = os.path.join(self.cache_dir, filename) - - try: - # Get file age - file_mtime = os.path.getmtime(file_path) - file_age_days = (current_time - file_mtime) / 86400 # Convert to days + # Collect files to process under lock + with self._lock: + files_to_check = [f for f in os.listdir(self.cache_dir) if f.endswith('.json')] + + for filename in files_to_check: + stats['files_scanned'] += 1 + file_path = os.path.join(self.cache_dir, filename) + + try: + # Get file age + file_mtime = os.path.getmtime(file_path) + file_age_days = (current_time - file_mtime) / 86400 # Convert to days
273-273: Consider validatingcache_strategyparameter.The
cache_strategyparameter is typed asAnyand used without validation. If passed incorrectly (e.g.,None), line 319 will raiseAttributeError. Adding a guard or typing it more specifically would improve robustness.- def cleanup_expired_files(self, cache_strategy: Any, retention_policies: Dict[str, int]) -> Dict[str, Any]: + def cleanup_expired_files(self, cache_strategy: 'CacheStrategy', retention_policies: Dict[str, int]) -> Dict[str, Any]:Or add a defensive check:
if cache_strategy is None or not hasattr(cache_strategy, 'get_data_type_from_key'): self.logger.warning("Invalid cache_strategy provided for cleanup") return {'files_scanned': 0, 'files_deleted': 0, 'space_freed_bytes': 0, 'errors': 0}src/cache/cache_metrics.py (1)
77-91: Moveimport timeto module level.The
import timeon line 86 is inside the method. While this works, it's non-idiomatic Python and incurs a small overhead on each call. Move it to the top of the file with other imports.♻️ Suggested fix
Add at the top of the file:
import timeThen remove the import from the method:
def record_disk_cleanup(self, files_cleaned: int, space_freed_mb: float, duration_sec: float) -> None: """ Record disk cleanup operation results. Args: files_cleaned: Number of files deleted space_freed_mb: Space freed in megabytes duration_sec: Duration of cleanup operation in seconds """ - import time with self._lock: self._metrics['last_disk_cleanup'] = time.time()src/cache_manager.py (1)
647-675: Consider adding graceful shutdown support for the cleanup thread.The cleanup thread runs an infinite loop with a 24-hour sleep. While
daemon=Trueensures it won't block process exit, there's no way to stop the thread gracefully for testing or controlled shutdown scenarios.Using a
threading.Eventwould allow cleaner lifecycle management and make the code more testable.♻️ Suggested improvement
+ self._cleanup_stop_event = threading.Event() def cleanup_loop(): """Background loop that runs cleanup periodically.""" self.logger.info("Disk cache cleanup thread started (interval: %d hours)", self._disk_cleanup_interval_hours) - while True: + while not self._cleanup_stop_event.is_set(): try: # Sleep for the configured interval sleep_seconds = self._disk_cleanup_interval_hours * 3600 - time.sleep(sleep_seconds) + # Use wait() instead of sleep() for interruptible sleep + if self._cleanup_stop_event.wait(timeout=sleep_seconds): + break # Stop event was set # Run cleanup self.logger.debug("Running scheduled disk cache cleanup") self.cleanup_disk_cache()Then add a stop method:
def stop_cleanup_thread(self) -> None: """Stop the background cleanup thread.""" if self._cleanup_stop_event: self._cleanup_stop_event.set()
The basketball scoreboard plugin uses sport_key="ncaam" and "ncaaw", but LogoDownloader only had "ncaam_basketball" mapped. This caused get_logo_directory() to fall back to "assets/sports/ncaam_logos" (non-existent) instead of "assets/sports/ncaa_logos". Added aliases to both LOGO_DIRECTORIES and API_ENDPOINTS: - ncaam -> assets/sports/ncaa_logos - ncaaw -> assets/sports/ncaa_logos - ncaaw_basketball -> assets/sports/ncaa_logos Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ChuckBuilds
force-pushed
the
fix/basketball-ncaa-logo-resolution
branch
from
625fd8d to
7a33767
Compare
Summary
sport_key="ncaam"and"ncaaw", butLogoDownloaderonly had"ncaam_basketball"mappedget_logo_directory()to fall back toassets/sports/ncaam_logos(non-existent) instead ofassets/sports/ncaa_logosChanges
Added aliases to both
LOGO_DIRECTORIESandAPI_ENDPOINTSinsrc/logo_downloader.py:ncaam->assets/sports/ncaa_logosncaaw->assets/sports/ncaa_logosncaaw_basketball->assets/sports/ncaa_logos(for consistency)Test plan
assets/sports/ncaa_logos🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.