From 51e6c273c47874775aa02b323ac8037b2774c7f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 17:36:37 +0000 Subject: [PATCH] fix: repair broken helper paths across display, cache, odds, logging, resolver, repos, config, validator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine fixes for bugs surfaced while writing coverage for previously untested modules (plus the bool-duration quirk pinned in PR #441): - base_plugin.get_display_duration: exclude bools from both numeric branches — display_duration=True no longer reads as a 1-second slot; it falls through to config, then the 15.0 default. - display_helper: draw_error_message/draw_no_data_message called _draw_centered_text with the wrong arguments and crashed with AttributeError — both now delegate to draw_centered_text. draw_scorebug_layout drew status and clock at the same y, overprinting each other — they now share one combined top line. draw_ticker_layout drew its text starting at x=display_width (fully off-canvas), returning a blank frame every time — now draws at x=0; scroll_speed stays accepted-but-unused and is documented as such. - api_helper.clear_cache guarded on a nonexistent CacheManager.clear() method, silently never clearing anything; it now uses the real surface (clear_cache/delete/list_cache_files) and no-ops safely otherwise. - base_odds_manager._extract_espn_data raised AttributeError when ESPN sent explicit JSON nulls ("homeTeamOdds": null) — every level now null-safes with 'or {}'. format_odds_summary gated on is_odds_available, which deliberately ignores money lines, so ML-only odds formatted as "No odds available" — it now gates only on empty/no_odds data and formats money lines. - logging_config.ContextualFormatter mutated record.msg in place, so a second handler prepended the context prefix twice; it now formats a copy. log_error hardcoded exc_info=True and raised TypeError when the caller passed exc_info — now kwargs.setdefault. - dynamic_team_resolver wrote its "shared" class cache through self, creating instance shadows — the cache was per-instance and every scoreboard refetched rankings. Writes now go through the class. - saved_repositories cleaned URLs with an unanchored .replace('.git','') that mangled URLs merely containing '.git' (my.github.io -> myhub.io); now strips only a trailing suffix. add/remove also roll back the in-memory list when the save fails, so memory always matches disk. - config_helper.merge_configs shallow-copied the base, aliasing every un-overridden nested dict into the result — now deep-copies. - startup_validator.validate_all accumulated errors/warnings across calls — now resets both lists per run. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --- src/base_odds_manager.py | 26 ++++++++---- src/common/api_helper.py | 39 ++++++++++++------ src/common/config_helper.py | 9 +++-- src/common/display_helper.py | 53 ++++++++++--------------- src/dynamic_team_resolver.py | 20 ++++++---- src/logging_config.py | 27 ++++++++----- src/plugin_system/base_plugin.py | 11 +++-- src/plugin_system/saved_repositories.py | 52 ++++++++++++++++-------- src/startup_validator.py | 7 +++- test/test_base_plugin_duration.py | 22 ++++++++-- 10 files changed, 170 insertions(+), 96 deletions(-) diff --git a/src/base_odds_manager.py b/src/base_odds_manager.py index 3520ce67..f35c4d54 100644 --- a/src/base_odds_manager.py +++ b/src/base_odds_manager.py @@ -163,19 +163,25 @@ class BaseOddsManager: item = data["items"][0] self.logger.debug(f"First item keys: {list(item.keys())}") - # The ESPN API returns odds data directly in the item, not in a providers array - # Extract the odds data directly from the item + # The ESPN API returns odds data directly in the item, not in a + # providers array. ESPN sends explicit JSON nulls for absent + # sides ("homeTeamOdds": null), so every level uses `or {}` — + # .get's default only applies when the key is missing entirely. + home = item.get("homeTeamOdds") or {} + away = item.get("awayTeamOdds") or {} extracted_data = { "details": item.get("details"), "over_under": item.get("overUnder"), "spread": item.get("spread"), "home_team_odds": { - "money_line": item.get("homeTeamOdds", {}).get("moneyLine"), - "spread_odds": item.get("homeTeamOdds", {}).get("current", {}).get("pointSpread", {}).get("value") + "money_line": home.get("moneyLine"), + "spread_odds": ((home.get("current") or {}) + .get("pointSpread") or {}).get("value") }, "away_team_odds": { - "money_line": item.get("awayTeamOdds", {}).get("moneyLine"), - "spread_odds": item.get("awayTeamOdds", {}).get("current", {}).get("pointSpread", {}).get("value") + "money_line": away.get("moneyLine"), + "spread_odds": ((away.get("current") or {}) + .get("pointSpread") or {}).get("value") } } self.logger.debug(f"Returning extracted odds data: {json.dumps(extracted_data, indent=2)}") @@ -260,9 +266,13 @@ class BaseOddsManager: Returns: Formatted odds summary string """ - if not self.is_odds_available(odds_data): + # Gate only on truly-empty / negative-cached data. is_odds_available + # deliberately ignores money lines (its callers decide whether to + # RENDER an odds widget), but a summary of money-line-only odds is + # still meaningful — the parts loop below handles them. + if not odds_data or odds_data.get('no_odds'): return "No odds available" - + parts = [] # Add spread information diff --git a/src/common/api_helper.py b/src/common/api_helper.py index fa9b7694..d6974b22 100644 --- a/src/common/api_helper.py +++ b/src/common/api_helper.py @@ -272,20 +272,35 @@ class APIHelper: def clear_cache(self, pattern: Optional[str] = None) -> None: """ Clear cache data. - + + Uses CacheManager's real surface (clear_cache / delete / + list_cache_files); safely no-ops on managers without it. The old + implementation guarded on a nonexistent ``clear`` method, so it + silently never cleared anything. + Args: - pattern: Optional pattern to match cache keys + pattern: Optional substring to match cache keys; only matching + entries are deleted. """ - if self.cache_manager: - if hasattr(self.cache_manager, 'clear'): - if pattern: - # Clear only keys matching pattern - keys = self.cache_manager.keys() - for key in keys: - if pattern in key: - self.cache_manager.delete(key) - else: - self.cache_manager.clear() + if not self.cache_manager: + return + if pattern: + if (hasattr(self.cache_manager, 'list_cache_files') + and hasattr(self.cache_manager, 'delete')): + for entry in self.cache_manager.list_cache_files(): + key = entry.get('key') if isinstance(entry, dict) else None + if key and pattern in key: + self.cache_manager.delete(key) + else: + self.logger.debug( + "Cache manager lacks list_cache_files/delete; " + "cannot clear by pattern") + elif hasattr(self.cache_manager, 'clear_cache'): + self.cache_manager.clear_cache() + elif hasattr(self.cache_manager, 'clear'): + self.cache_manager.clear() + else: + self.logger.debug("Cache manager exposes no clear method; no-op") def _get_from_cache(self, key: str) -> Optional[Any]: """Get data from cache.""" diff --git a/src/common/config_helper.py b/src/common/config_helper.py index 79840188..319af082 100644 --- a/src/common/config_helper.py +++ b/src/common/config_helper.py @@ -5,6 +5,7 @@ Handles configuration management and validation for LED matrix plugins. Extracted from LEDMatrix core to provide reusable functionality for plugins. """ +import copy import json import logging from pathlib import Path @@ -160,10 +161,12 @@ class ConfigHelper: override_config: Configuration to merge in (takes precedence) Returns: - Merged configuration dictionary + Merged configuration dictionary (fully independent of both + inputs — a shallow copy would alias un-overridden nested dicts, + so mutating the result would mutate the caller's base config). """ - merged = base_config.copy() - + merged = copy.deepcopy(base_config) + for key, value in override_config.items(): if key in merged and isinstance(merged[key], dict) and isinstance(value, dict): # Recursively merge nested dictionaries diff --git a/src/common/display_helper.py b/src/common/display_helper.py index 13dc3030..c88c9e63 100644 --- a/src/common/display_helper.py +++ b/src/common/display_helper.py @@ -115,17 +115,13 @@ class DisplayHelper: if home_logo and away_logo: self._draw_logos(main_img, home_logo, away_logo) - # Draw status/period text (top center) - if status_text or period_text: - status_display = f"{period_text} {status_text}".strip() - if status_display: - self._draw_centered_text(draw, status_display, - fonts.get('time', fonts.get('status')), - y_position=1) - - # Draw clock if available - if clock: - self._draw_centered_text(draw, clock, fonts.get('time'), y_position=1) + # Draw one combined top line (period/status/clock all share y=1 — + # drawing them separately overprinted each other). + top_line = " ".join(p for p in [period_text, status_text, clock] if p) + if top_line: + self._draw_centered_text(draw, top_line, + fonts.get('time', fonts.get('status')), + y_position=1) # Draw scores (center) score_text = f"{away_score}-{home_score}" @@ -153,26 +149,28 @@ class DisplayHelper: """ Draw a ticker/scrolling text layout. + Renders a single static frame with the text at the left edge; the + caller advances the scroll by re-rendering or shifting. The + scroll_speed parameter is accepted for API compatibility but does + not affect this frame. (Previously the text was drawn starting at + x=display_width — entirely off-canvas — so every frame was blank.) + Args: text: Text to display font: Font to use background_color: Background color text_color: Text color - scroll_speed: Pixels to scroll per frame - + scroll_speed: Accepted for compatibility; unused per-frame + Returns: PIL Image with ticker layout """ img = self.create_base_image(background_color) draw = ImageDraw.Draw(img) - - # Start text off-screen to the right - x_position = self.display_width - - # Draw text - self._draw_text_with_outline(draw, text, (x_position, self.display_height // 2 - 6), + + self._draw_text_with_outline(draw, text, (0, self.display_height // 2 - 6), font, fill=text_color) - + return img def draw_centered_text(self, text: str, font: ImageFont.ImageFont, @@ -214,15 +212,9 @@ class DisplayHelper: Returns: PIL Image with error message """ - img = self.create_base_image((50, 0, 0)) # Dark red background - - # Use default font + # Dark red background, white text font = ImageFont.load_default() - - # Draw centered error message - self._draw_centered_text(message, font, (50, 0, 0), (255, 255, 255)) - - return img + return self.draw_centered_text(message, font, (50, 0, 0), (255, 255, 255)) def draw_no_data_message(self, message: str = "No Data") -> Image.Image: """ @@ -234,11 +226,8 @@ class DisplayHelper: Returns: PIL Image with no data message """ - img = self.create_base_image((0, 0, 0)) font = ImageFont.load_default() - self._draw_centered_text(message, font, (0, 0, 0), (150, 150, 150)) - - return img + return self.draw_centered_text(message, font, (0, 0, 0), (150, 150, 150)) def get_display_dimensions(self) -> Tuple[int, int]: """ diff --git a/src/dynamic_team_resolver.py b/src/dynamic_team_resolver.py index ab969e17..7cbe09cd 100644 --- a/src/dynamic_team_resolver.py +++ b/src/dynamic_team_resolver.py @@ -167,10 +167,14 @@ class DynamicTeamResolver: # Sort by ranking (1, 2, 3, etc.) sorted_rankings = dict(sorted(rankings.items(), key=lambda x: x[1])) - - # Cache the results - self._rankings_cache = sorted_rankings - self._cache_timestamp = current_time + + # Cache the results ON THE CLASS. Assigning through self + # would create instance attributes that shadow the shared + # class-level cache, making it per-instance — and every + # scoreboard constructs its own resolver, so the cache + # would never actually be shared. + DynamicTeamResolver._rankings_cache = sorted_rankings + DynamicTeamResolver._cache_timestamp = current_time self.logger.info(f"Fetched rankings for {len(sorted_rankings)} teams") return sorted_rankings @@ -216,9 +220,11 @@ class DynamicTeamResolver: return any(pattern in team_name.upper() for pattern in dynamic_patterns) def clear_cache(self): - """Clear the rankings cache to force fresh data on next request.""" - self._rankings_cache = {} - self._cache_timestamp = 0 + """Clear the SHARED rankings cache to force fresh data on next + request. Writes through the class — assigning via self would only + shadow the shared cache for this instance.""" + DynamicTeamResolver._rankings_cache = {} + DynamicTeamResolver._cache_timestamp = 0 self.logger.info("Cleared dynamic team rankings cache") diff --git a/src/logging_config.py b/src/logging_config.py index 48eba67c..17d831bb 100644 --- a/src/logging_config.py +++ b/src/logging_config.py @@ -5,6 +5,7 @@ Provides consistent logging configuration across the LEDMatrix application. Supports structured logging with context information and appropriate log levels. """ +import copy import logging import sys import os @@ -65,24 +66,29 @@ class ContextualFormatter(logging.Formatter): self.include_context = include_context def format(self, record: logging.LogRecord) -> str: - """Format log record with context.""" - # Add context to message if present + """Format log record with context. + + Works on a shallow copy of the record: a record is formatted once + PER HANDLER, so mutating record.msg in place (the old behavior) + prepended the context prefix again for every additional handler. + """ if self.include_context: context_parts = [] - + if hasattr(record, 'plugin_id'): context_parts.append(f"[Plugin: {record.plugin_id}]") - + if hasattr(record, 'operation_id'): context_parts.append(f"[Op: {record.operation_id}]") - + if hasattr(record, 'context') and isinstance(record.context, dict): for key, value in record.context.items(): context_parts.append(f"[{key}: {value}]") - + if context_parts: + record = copy.copy(record) record.msg = ' '.join(context_parts) + ' ' + str(record.msg) - + return super().format(record) @@ -224,8 +230,11 @@ def log_warning(logger: logging.Logger, message: str, **kwargs) -> None: def log_error(logger: logging.Logger, message: str, **kwargs) -> None: - """Log error message with context.""" - log_with_context(logger, logging.ERROR, message, **kwargs, exc_info=True) + """Log error message with context. Defaults exc_info=True; a caller + passing exc_info explicitly wins (the old hardcoded keyword raised + TypeError on that duplicate).""" + kwargs.setdefault('exc_info', True) + log_with_context(logger, logging.ERROR, message, **kwargs) def log_debug(logger: logging.Logger, message: str, **kwargs) -> None: diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index 8ec4414d..e821bbc4 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -364,8 +364,10 @@ class BasePlugin(ABC): # Handle None case if duration is None: pass # Fall through to config - # Try to convert to float if it's a number or numeric string - elif isinstance(duration, (int, float)): + # Try to convert to float if it's a number or numeric string. + # bool is excluded: it's an int subclass, and True would + # otherwise read as a 1-second duration. + elif isinstance(duration, (int, float)) and not isinstance(duration, bool): if duration > 0: return float(duration) else: @@ -403,8 +405,9 @@ class BasePlugin(ABC): # Fall back to config config_duration = self.config.get("display_duration", 15.0) try: - # Ensure config value is also a valid float - if isinstance(config_duration, (int, float)): + # Ensure config value is also a valid float (bool excluded — an + # int subclass that would otherwise read True as 1 second) + if isinstance(config_duration, (int, float)) and not isinstance(config_duration, bool): if config_duration > 0: return float(config_duration) else: diff --git a/src/plugin_system/saved_repositories.py b/src/plugin_system/saved_repositories.py index bd0e1872..9635437a 100644 --- a/src/plugin_system/saved_repositories.py +++ b/src/plugin_system/saved_repositories.py @@ -57,6 +57,18 @@ class SavedRepositoriesManager: self.logger.error(f"Error saving repositories: {e}") return False + @staticmethod + def _clean_url(repo_url: str) -> str: + """Normalize a repo URL: strip whitespace, trailing slashes, and a + trailing ``.git`` suffix ONLY. (The old ``.replace('.git', '')`` + was an unanchored substring replace that mangled URLs merely + containing ``.git``, e.g. ``https://github.com/user/my.github.io``.) + """ + repo_url = repo_url.strip().rstrip('/') + if repo_url.endswith('.git'): + repo_url = repo_url[:-4] + return repo_url + def get_all(self) -> List[Dict[str, str]]: """Get all saved repositories.""" return self.repositories.copy() @@ -72,15 +84,14 @@ class SavedRepositoriesManager: Returns: True if added successfully """ - # Clean URL - repo_url = repo_url.strip().rstrip('/').replace('.git', '') - + repo_url = self._clean_url(repo_url) + # Check if already exists for repo in self.repositories: if repo.get('url') == repo_url: self.logger.warning(f"Repository already exists: {repo_url}") return False - + # Extract name from URL if not provided if not name: parts = repo_url.split('/') @@ -88,15 +99,20 @@ class SavedRepositoriesManager: name = parts[-1] else: name = repo_url - + # Add repository self.repositories.append({ 'url': repo_url, 'name': name, 'type': 'registry' if 'plugins.json' in repo_url or 'ledmatrix-plugins' in repo_url.lower() else 'single' }) - - return self._save_repositories() + + if not self._save_repositories(): + # Keep memory consistent with disk: a failed save must not leave + # a phantom entry that only this process can see. + self.repositories.pop() + return False + return True def remove(self, repo_url: str) -> bool: """ @@ -108,21 +124,25 @@ class SavedRepositoriesManager: Returns: True if removed successfully """ - # Clean URL - repo_url = repo_url.strip().rstrip('/').replace('.git', '') - - original_count = len(self.repositories) - self.repositories = [r for r in self.repositories if r.get('url') != repo_url] - - if len(self.repositories) < original_count: - return self._save_repositories() + repo_url = self._clean_url(repo_url) + + previous = self.repositories + remaining = [r for r in previous if r.get('url') != repo_url] + + if len(remaining) < len(previous): + self.repositories = remaining + if not self._save_repositories(): + # Failed save: restore so memory matches disk. + self.repositories = previous + return False + return True else: self.logger.warning(f"Repository not found: {repo_url}") return False def has(self, repo_url: str) -> bool: """Check if a repository is already saved.""" - repo_url = repo_url.strip().rstrip('/').replace('.git', '') + repo_url = self._clean_url(repo_url) return any(r.get('url') == repo_url for r in self.repositories) def get_registry_repositories(self) -> List[Dict[str, str]]: diff --git a/src/startup_validator.py b/src/startup_validator.py index 86c703f6..8dd9fce2 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -37,7 +37,12 @@ class StartupValidator: Tuple of (is_valid, errors, warnings) """ self.logger.info("Starting startup validation...") - + + # Fresh lists each run — without this, calling validate_all() twice + # duplicated every message. + self.errors = [] + self.warnings = [] + # Validate configuration self._validate_config() diff --git a/test/test_base_plugin_duration.py b/test/test_base_plugin_duration.py index 4039e1fc..ffe03153 100644 --- a/test/test_base_plugin_duration.py +++ b/test/test_base_plugin_duration.py @@ -78,10 +78,20 @@ class TestInstanceVariable: instance_duration=[30]) assert plugin.get_display_duration() == 20.0 - def test_bool_true_is_one_second(self): - # Characterized quirk: bool is an int subclass, so display_duration = - # True passes the isinstance((int, float)) branch and returns 1.0. - assert make_plugin(instance_duration=True).get_display_duration() == 1.0 + def test_bool_true_falls_through_like_any_non_number(self): + # bool is an int subclass, but a boolean is not a duration: True + # must NOT read as 1 second — it falls through to config/default. + assert make_plugin(instance_duration=True).get_display_duration() == 15.0 + + def test_bool_true_falls_through_to_config(self): + plugin = make_plugin(config={"display_duration": 20}, + instance_duration=True) + assert plugin.get_display_duration() == 20.0 + + def test_bool_false_still_falls_through(self): + plugin = make_plugin(config={"display_duration": 20}, + instance_duration=False) + assert plugin.get_display_duration() == 20.0 class TestConfigFallback: @@ -108,3 +118,7 @@ class TestConfigFallback: def test_config_none_uses_default(self): assert make_plugin({"display_duration": None}).get_display_duration() == 15.0 + + def test_config_bool_uses_default(self): + assert make_plugin({"display_duration": True}).get_display_duration() == 15.0 + assert make_plugin({"display_duration": False}).get_display_duration() == 15.0