Sports unification phases 1–2: package split, promoted methods, opt-in capabilities - #426
Conversation
FontManager built its catalog from cwd-relative paths ('assets/fonts'),
so any process started outside the install root — the plugin safety
harness on CI being the recurring case — found no fonts and silently
degraded every plugin to PIL's default face. Several plugins grew
per-plugin workarounds for exactly this (countdown, text-display,
tide-display in the plugins monorepo).
Catalog population now falls back to the install root derived from this
module's location when the cwd-relative path is missing; behavior when
running from the install root is unchanged. Verified: resolve_font
returns the real FreeType face from a foreign cwd, and the full unit
suites (266 tests) pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
The plugins monorepo's sunset rule ('delete a bundled fallback copy only
when the manifest floors on the first core release shipping the module')
needs core module additions recorded against version numbers. Seeds the
changelog at 3.1.0 and documents the discipline.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
The existing workflow ran only the three plugin-harness suites; the skin-system, font-manager, data-source, extractor, scroll-helper, adaptive-layout, and loader-compat suites (266 tests) existed but never ran in CI, so a refactor of src/base_classes or src/common could regress them silently. Also enrolls the new sports characterization and element-style suites landing in this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
… already expect Three plugins (of-the-day, ledmatrix-music, football-scoreboard) import src.element_style behind guarded try/except with classic fallbacks, but the module never existed in core, so the richer per-element styling UI those code paths implement has been dormant. This lands it: - ElementStyleResolver.style() resolves per-element font/size/color with the key semantic the consumers encode: a config value counts as user-forced only when it differs from the schema default (the web UI bakes defaults into config.json on save), and untouched configs resolve to exactly the caller's classic values — byte-identical rendering, proven by of-the-day's committed goldens passing unchanged. - defaults_from_schema_file parses both declaration forms (the compact x-style-elements map and hand-written customization blocks). - expand_style_elements() expands x-style-elements into full config blocks; schema_manager.load_schema() applies it (guarded, no-op for schemas without the declaration) so the config form and defaults merging see the expanded UI. - Fonts resolve cwd-independently with (path, size) caching; .bdf loads via freetype like FontManager; nothing in the module raises out of style(). Verified: 31 new unit tests; of-the-day's previously-skipped 9-test spec suite now runs and passes; football's resolver tests pass (27); music's 38 plugin tests pass; schema-manager suites pass (43). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
…unification Pins current behavior before the planned merge of the nine drifted plugin copies back into this ancestor: the _extract_game_details_common key contract per sport (reusing GUARANTEED_KEYS from the skin tests), update() flows for upcoming/recent/live against cache-seeded fixtures under frozen time, rendering smoke per mode class, and guard rails on the skin-system seam. Five surprising behaviors are pinned AS-IS and flagged in comments so the merge changes them knowingly or not at all: is_upcoming also matching status.type.name; hockey dropping events whose competitors lack 'statistics'; baseball reading the event-level status for innings; no past-date filter in upcoming; and favorites-only mode with an empty favorites list showing nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
CodeQL flagged the new unit-tests job for running with the default unrestricted token; the pre-existing job had the same exposure. Both jobs only check out the repo and run pytest, so a workflow-level contents:read is sufficient. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
Phase B1a of docs/SPORTS_UNIFICATION.md. src/base_classes/sports.py becomes a package so the upcoming capability modules have a home and diffs show their blast radius: sports/__init__.py re-exports the public API sports/core.py SportsCore sports/modes.py SportsUpcoming / SportsRecent / SportsLive No logic change: the 1515 class-body lines are byte-identical to the original (verified by concatenating the two modules and diffing against HEAD). Only module docstrings and the redistributed import blocks are new. MRO and __abstractmethods__ are unchanged, and every existing import site — including 'from src.base_classes.sports import SportsCore' in the sport subclasses, the skin tests, and the characterization suite — resolves through the package __init__. One test edit was required: the characterization suite monkeypatched 'src.base_classes.sports.get_background_service', which is no longer a module attribute on a package. Retargeted to 'src.base_classes.sports.core.get_background_service' — the module whose globals SportsCore.__init__ actually resolves, so the patch is effective exactly as before. No test logic or assertion changed. Also adds docs/SPORTS_UNIFICATION.md: the architecture for the whole B1-B5 sequence — how upgradability (guarded imports, capability probing, frozen view-model keys, the sunset rule), reusability (promote only what all nine copies share), and modularity (capabilities as opt-in mixins rather than config branches, variants as named strategies, sport-unique code as declared override points) are kept as three separate mechanisms. Verified: characterization + skin 94 passed; the 10-file unit suite 338 passed; test/plugins 60 passed — all identical to pre-change counts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
Phase B1b of docs/SPORTS_UNIFICATION.md. Every method here is present in
all nine bundled plugin sports.py copies and absent from core, so this is
reuse of code the fleet already agreed on — not new behavior. The
promotions are inert until B5: the plugins' own overrides still run.
SportsCore: cleanup, _get_layout_offset, _load_custom_font_from_element_config
SportsUpcoming: _select_games_for_display
SportsRecent: _get_zero_clock_duration, _clear_zero_clock_tracking,
_select_recent_games_for_display
SportsLive: _is_game_really_over, _detect_stale_games
Where the copies disagreed, the canonical form was chosen on evidence and
the genuine per-sport differences became seams rather than branches:
- _favorite_key(game, side) -- NRL matches favorites on team id because its
abbreviations are ambiguous (NEW is both Newcastle Knights and New
Zealand Warriors). Default is the abbreviation; NRL overrides. Core never
learns the string nrl.
- FINAL_PERIOD / CLOCK_COUNTS_DOWN -- hockey ends in P3, and soccer/afl/nrl
clocks count UP, so 0:00 means kickoff, not expiry.
- _config_schema_path() / _font_root() -- plugin-supplied locations, never
derived from this module's __file__.
BEHAVIOR CHANGE (baseball, ufc): the rejected variant coerced a missing or
non-str clock to the literal 0:00 and then declared the game over at
period >= 4. MLB has no game clock and period is the inning, so live games
were being evicted from the 5th inning onward; UFC likewise. The promoted
variant skips the clock check when the clock is unusable -- it fails safe
(keeps showing the game) instead of failing destructive.
Also fixes a regression from the package move in e591cec: the bodies were
byte-identical but __file__ gained a directory, so _resolve_project_path's
parents[2] silently began resolving to <root>/src instead of the repo root.
Both it and _font_root now derive from a single _INSTALL_ROOT constant, so
a future move needs one line changed rather than two hand-counted depths.
Tests assert the resolved values, not the index.
The font loader takes baseball's body (BDF memo cache + native-strike
retry) under hockey's Optional signature -- the older lineage is the
correct one here, and basketball's positional str default breaks on an
explicit None. It resolves through _font_root rather than the cwd, so it
does not reintroduce the bug just fixed for FontManager, and delegates to
FontManager for the alias table and BDF header parse instead of shipping
second copies. cleanup gained the two new font caches and still leaves
background_service alone -- it is a process-wide singleton.
Verified: 111 new tests (48 core + 59 modes + 4 install-root regression);
characterization + skin suites still exactly 94, unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
…d keys Both bugs were pinned AS-IS by the B0 characterization suite so this phase could change them knowingly. Both fixes are adoptions of code the corresponding plugins already ship, not new inventions. Hockey: the extractor read competitor["statistics"] unguarded, so a competitor arriving without that array raised KeyError inside the generator and the WHOLE event was discarded -- valid scores and status included. Shot/save counts now default to 0, which is already what the suite expects for an empty statistics array. Baseball: for live games the extractor read game_event["status"], the event TOP-LEVEL status, to get the inning. Real ESPN events duplicate status there, but MiLB events (synthesized from the MLB Stats API into an ESPN-like shape) populate only the competition-level one, so the lookup raised a bare KeyError and dropped the event. It now reads the competition-level status that _extract_game_details_common has already validated, so it cannot be missing at that point. The two characterization tests that pinned the old behaviour are rewritten to assert the fix rather than deleted, so the suite still documents the edge case -- and still totals 94. CHANGELOG records these plus the live-clock change from aaabc61 under Changed/Fixed, since all three are user-visible. The two new promotion suites join the CI unit job (449 tests). Verified: unit job 449 passed, plugin-safety job 60 passed, and the hockey (16) and baseball (24) plugin harnesses render clean at every panel size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe sports module becomes a package with shared core and mode classes. The PR adds celebration, rotation, and scrolling capabilities. It fixes incomplete baseball and hockey data handling and adds tests, documentation, workflow coverage, and the 3.2.0 version update. ChangesSports unification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SportsPlugin
participant SportsCore
participant SportsLive
participant SportsScrollDisplayManager
SportsPlugin->>SportsCore: request shared scoreboard behavior
SportsCore->>SportsLive: provide normalized game details
SportsLive->>SportsLive: evaluate clock, period, and freshness
SportsPlugin->>SportsScrollDisplayManager: prepare and display game content
SportsScrollDisplayManager->>SportsScrollDisplayManager: advance and render frames
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 632 |
| Duplication | 37 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@coderabbitai review (Auto-review is skipped here because this PR is stacked on #425's branch rather than Generated by Claude Code |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
src/base_classes/sports/__init__.py (1)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__to satisfy Ruff RUF022.Ruff reports
__all__is not sorted. If RUF022 is enabled in the project configuration, the lint job fails.♻️ Proposed fix
__all__ = [ "SportsCore", - "SportsUpcoming", - "SportsRecent", "SportsLive", + "SportsRecent", + "SportsUpcoming", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/__init__.py` around lines 12 - 17, Sort the entries in the __all__ declaration alphabetically to satisfy Ruff RUF022, preserving the same exported symbols and their string values.Source: Linters/SAST tools
docs/SPORTS_UNIFICATION.md (1)
70-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced block and mark the not-yet-shipped paths.
Line 70 opens a fenced block with no language, which markdownlint flags as MD040. The tree also lists
capabilities/celebrations.pyandcapabilities/rotation.py, which arrive in phase B2. A reader of the current tree will not find them. Mark them as planned.📝 Proposed documentation fix
-``` +```text src/base_classes/sports/ __init__.py re-exports the public API (import path unchanged) core.py SportsCore — fetch, cache, config, logos, fonts, odds, view-model extraction, the skin seam modes.py SportsUpcoming / SportsRecent / SportsLive - capabilities/ - celebrations.py CelebrationMixin (opt-in: 4 of 9 plugins) - rotation.py RotationStrategy + registry + capabilities/ (planned, phase B2 — not yet present) + celebrations.py CelebrationMixin (opt-in: 4 of 9 plugins) + rotation.py RotationStrategy + registry</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/SPORTS_UNIFICATION.mdaround lines 70 - 79, Update the fenced tree
block in SPORTS_UNIFICATION.md to declare the text language for Markdown lint
compliance, and annotate the capabilities directory as planned for phase B2 and
not yet present. Keep celebrations.py and rotation.py listed beneath that
planned directory without changing their descriptions.</details> <!-- cr-comment:v1:7be8a36d45e630637f94122e --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>src/base_classes/sports/core.py (4)</summary><blockquote> `692-693`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Use `self.logger` in `_extract_game_details_common`.** Lines 693 and 771 log through the module-level `logging` root logger. Every other error path in this class uses `self.logger`. On the Raspberry Pi the root-logger records lose the manager context and can bypass the configured handlers, which makes extraction failures hard to trace remotely. <details> <summary>♻️ Proposed fix</summary> ```diff - logging.warning(f"Could not parse game date: {game_date_str}") + self.logger.warning( + f"[SportsCore] Could not parse game date '{game_date_str}' for event " + f"{game_event.get('id')}; start time will be missing" + )- logging.error(f"Error extracting game details: {e} from event: {game_event.get('id')}", exc_info=True) + self.logger.error( + f"[SportsCore] Error extracting game details from event " + f"{game_event.get('id')}: {e}", + exc_info=True, + )As per coding guidelines: "Use structured logging with context (e.g.,
[NHL Recent]) for logging messages".Also applies to: 769-772
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/core.py` around lines 692 - 693, Update the warning handlers in _extract_game_details_common, including the ValueError paths around the game-date parsing and lines 769–772, to use self.logger instead of the module-level logging root logger. Preserve the existing warning level and include the appropriate manager context in the message, such as the established “[NHL Recent]” structured prefix.Source: Coding guidelines
1074-1082: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAlso release the rankings cache and the loaded skin in
cleanup.
cleanupclears the logo and font caches. It leaves_team_rankings_cacheand_skinpopulated. The skin object holds its own loaded assets. On the Raspberry Pi these survive every enable/disable cycle of the plugin and add to the RAM footprint.♻️ Proposed fix
if hasattr(self, '_bdf_native_size_cache'): self._bdf_native_size_cache.clear() + if hasattr(self, '_team_rankings_cache'): + self._team_rankings_cache.clear() + self._rankings_cache_timestamp = 0 + # Drop the loaded skin so its assets are collected; it reloads lazily. + self._skin = None + self._skin_load_attempted = FalseAs per coding guidelines: "Clean up resources regularly to manage memory effectively".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/core.py` around lines 1074 - 1082, Update cleanup to also release the resources held by _team_rankings_cache and _skin, clearing or resetting them using the same guarded cleanup pattern as the existing logo and font caches. Ensure the loaded skin object and rankings data are no longer retained after cleanup.Source: Coding guidelines
929-934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid reading the private
_configattribute ofElementStyleResolverin cache invalidation.
SportsCoredepends onElementStyleResolver._configfor the swapped-config guard. If access moves behind a public accessor or the field is renamed, this lookup will raise instead of recreating the resolver. Store the previous config identity locally instead, without importing it from the resolver.♻️ Proposed fix
resolver = getattr(self, '_style_resolver_cached', None) - if resolver is None or resolver._config is not self.config: + cached_config = getattr(self, '_style_resolver_config', None) + if resolver is None or cached_config is not self.config: resolver = ElementStyleResolver( self.config, defaults_from_schema_file(schema_path)) self._style_resolver_cached = resolver + self._style_resolver_config = self.config return resolver.offset_value(element, axis, default)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/core.py` around lines 929 - 934, Update the resolver cache logic around _style_resolver_cached in SportsCore to track the previously used config identity locally, rather than reading ElementStyleResolver._config. Recreate and replace the cached resolver whenever self.config differs from the locally stored config, while preserving the existing offset_value call.
826-829: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
date_strmay be unbound in theexcepthandler.Line 828 formats
date_strin the handler.date_stris assigned at line 816. Anyrequestsexception raised before that assignment, for example a connection error surfaced by an adapter during session setup, producesUnboundLocalErrorand hides the original error. Initializedate_strbefore thetryblock.🛡️ Proposed fix
try: + date_str = "unknown" # Fetch current week and next few days for immediate display now = datetime.now(pytz.utc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/core.py` around lines 826 - 829, Initialize date_str before the try block in the surrounding game-fetching method so it is always bound when the RequestException handler logs through self.logger.warning. Preserve the existing date-specific assignment and return None behavior, ensuring early request failures retain their original error instead of raising UnboundLocalError.test/test_sports_core_promotions.py (2)
56-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth new fixtures leak the real
requests.Sessionopened bySportsCore.__init__. Constructing a concrete manager creates a liverequests.Sessionwith mounted retry adapters. Neither fixture closes it, so every built manager leaks a connection pool for the whole run.
test/test_sports_core_promotions.py#L56-L79: collect the built managers and closemanager.sessionin fixture teardown; convertbuildto ayieldfixture.test/test_sports_modes_promotions.py#L99-L133: close the originalmanager.sessionbefore line 128 rebinds it to aMagicMock, or share the same teardown helper.As per coding guidelines: "Clean setup and teardown between tests to reset state".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_sports_core_promotions.py` around lines 56 - 79, Both fixtures leak the real requests.Session created during SportsCore construction. In test/test_sports_core_promotions.py lines 56-79, convert build into a yield fixture, track every manager returned by _build, and close each manager.session during teardown. In test/test_sports_modes_promotions.py lines 99-133, close the original manager.session before rebinding it to MagicMock, or reuse the shared teardown helper; preserve the existing fixture behavior otherwise.Source: Coding guidelines
547-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tmp_pathto clear the hardcoded-tmp lint findings.Ruff reports
S108and ast-grep reportshardcoded-tmp-filefor"/tmp/some/logo/dir". The assertion only needs any absolute path, so thetmp_pathfixture removes the finding without changing what the test proves.♻️ Proposed change
- def test_absolute_paths_pass_through_unchanged(self): + def test_absolute_paths_pass_through_unchanged(self, tmp_path): from src.base_classes.sports.core import SportsCore - absolute = Path("/tmp/some/logo/dir") + absolute = tmp_path / "some" / "logo" / "dir" assert SportsCore._resolve_project_path(None, absolute) == absolute🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_sports_core_promotions.py` around lines 547 - 551, Update test_absolute_paths_pass_through_unchanged to accept the pytest tmp_path fixture and construct the absolute test path from tmp_path instead of the hardcoded "/tmp/some/logo/dir" literal, preserving the existing pass-through assertion.Source: Linters/SAST tools
src/base_classes/sports/modes.py (2)
462-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared selection body.
_select_recent_games_for_displayduplicates_select_games_for_display(lines 37-101). Only three things differ: the sort direction, the missing-time sentinel, and the per-team limit. A single private helper onSportsCore— for example_select_by_favorite_quota(games, favorite_teams, limit, *, newest_first)— lets both mode classes keep their public names while sharing one body. That keeps thehasattrassertions intest/test_sports_modes_promotions.pysatisfied.♻️ Sketch of the delegation
def _select_recent_games_for_display( self, processed_games: List[Dict], favorite_teams: List[str] ) -> List[Dict]: - sorted_games = sorted( - processed_games, - key=lambda g: g.get("start_time_utc") - or datetime.min.replace(tzinfo=timezone.utc), - reverse=True, - ) - ... + return self._select_by_favorite_quota( + processed_games, + favorite_teams, + limit=self.recent_games_to_show, + newest_first=True, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/modes.py` around lines 462 - 526, Extract the shared favorite-team quota selection logic from `_select_recent_games_for_display` and `_select_games_for_display` into a private `SportsCore` helper such as `_select_by_favorite_quota`. Parameterize the helper for the per-team limit, sort direction, and missing-time sentinel, then have both existing methods delegate to it while preserving their public names and behavior, including the promotion tests’ `hasattr` expectations.
37-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire the promoted selectors into the public update paths.
update()still uses the old inline, multi-pass favorite-team selection, so_select_games_for_displayand_select_recent_games_for_displayare only exercised by tests. Call the promoted selectors inSportsUpcoming.update()andSportsRecent.update()before applying the final cap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/modes.py` around lines 37 - 101, Update the public update paths in SportsUpcoming.update() and SportsRecent.update() to call _select_games_for_display and _select_recent_games_for_display respectively, replacing the old inline multi-pass favorite-team selection. Apply each method’s final display cap only after the promoted selector returns, preserving existing behavior when no favorites are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/base_classes/baseball.py`:
- Around line 167-173: The favorite-team debug logging still accesses
event-level status directly, causing a KeyError when only competition-level
status exists. In src/base_classes/baseball.py lines 167-173, update the
remaining status log to reuse the validated status object from
_extract_game_details_common. In test/test_sports_base_characterization.py lines
350-364, pass favorites=["TB"] so the characterization test covers the
favorite-team path and missing event-level status.
In `@src/base_classes/sports/core.py`:
- Around line 427-446: The _load_fonts method must resolve all font files
through the existing _resolve_font_path helper instead of using
working-directory-relative paths, while preserving fallback behavior. Replace
module-level logging calls with self.logger calls so messages include manager
context, and add the method’s return type hint consistent with the returned font
mapping.
- Around line 623-629: Initialize _last_warning_time in SportsCore.__init__
before any callers can invoke _should_log, preserving the existing cooldown
behavior. If cooldown tracking is intended per warning type, replace the single
timestamp with warning_type-keyed state and update _should_log accordingly;
otherwise retain the global timestamp.
In `@src/base_classes/sports/modes.py`:
- Around line 886-901: Update the clock handling in _is_game_really_over to
safely handle a present-but-None period before comparing it with FINAL_PERIOD,
preserving non-expired behavior for missing or invalid periods. Expand the
zero-clock detection to recognize the normalized "00:00" form, ideally through a
digit-only zero check covering existing formats. Extend the relevant
parametrized tests, including "00:00" and a late-period period=None case.
- Around line 845-852: Update the SportsLive live-data ingestion path that
processes new_live_games to record each game’s last_seen timestamp in
self.game_update_timestamps, ensuring _detect_stale_games can evict games after
stale_game_timeout; add an explicit type hint matching _zero_clock_timestamps
for self.game_update_timestamps.
---
Nitpick comments:
In `@docs/SPORTS_UNIFICATION.md`:
- Around line 70-79: Update the fenced tree block in SPORTS_UNIFICATION.md to
declare the text language for Markdown lint compliance, and annotate the
capabilities directory as planned for phase B2 and not yet present. Keep
celebrations.py and rotation.py listed beneath that planned directory without
changing their descriptions.
In `@src/base_classes/sports/__init__.py`:
- Around line 12-17: Sort the entries in the __all__ declaration alphabetically
to satisfy Ruff RUF022, preserving the same exported symbols and their string
values.
In `@src/base_classes/sports/core.py`:
- Around line 692-693: Update the warning handlers in
_extract_game_details_common, including the ValueError paths around the
game-date parsing and lines 769–772, to use self.logger instead of the
module-level logging root logger. Preserve the existing warning level and
include the appropriate manager context in the message, such as the established
“[NHL Recent]” structured prefix.
- Around line 1074-1082: Update cleanup to also release the resources held by
_team_rankings_cache and _skin, clearing or resetting them using the same
guarded cleanup pattern as the existing logo and font caches. Ensure the loaded
skin object and rankings data are no longer retained after cleanup.
- Around line 929-934: Update the resolver cache logic around
_style_resolver_cached in SportsCore to track the previously used config
identity locally, rather than reading ElementStyleResolver._config. Recreate and
replace the cached resolver whenever self.config differs from the locally stored
config, while preserving the existing offset_value call.
- Around line 826-829: Initialize date_str before the try block in the
surrounding game-fetching method so it is always bound when the RequestException
handler logs through self.logger.warning. Preserve the existing date-specific
assignment and return None behavior, ensuring early request failures retain
their original error instead of raising UnboundLocalError.
In `@src/base_classes/sports/modes.py`:
- Around line 462-526: Extract the shared favorite-team quota selection logic
from `_select_recent_games_for_display` and `_select_games_for_display` into a
private `SportsCore` helper such as `_select_by_favorite_quota`. Parameterize
the helper for the per-team limit, sort direction, and missing-time sentinel,
then have both existing methods delegate to it while preserving their public
names and behavior, including the promotion tests’ `hasattr` expectations.
- Around line 37-101: Update the public update paths in SportsUpcoming.update()
and SportsRecent.update() to call _select_games_for_display and
_select_recent_games_for_display respectively, replacing the old inline
multi-pass favorite-team selection. Apply each method’s final display cap only
after the promoted selector returns, preserving existing behavior when no
favorites are configured.
In `@test/test_sports_core_promotions.py`:
- Around line 56-79: Both fixtures leak the real requests.Session created during
SportsCore construction. In test/test_sports_core_promotions.py lines 56-79,
convert build into a yield fixture, track every manager returned by _build, and
close each manager.session during teardown. In
test/test_sports_modes_promotions.py lines 99-133, close the original
manager.session before rebinding it to MagicMock, or reuse the shared teardown
helper; preserve the existing fixture behavior otherwise.
- Around line 547-551: Update test_absolute_paths_pass_through_unchanged to
accept the pytest tmp_path fixture and construct the absolute test path from
tmp_path instead of the hardcoded "/tmp/some/logo/dir" literal, preserving the
existing pass-through assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bcaf8d63-425b-4f95-9b5c-1edbb139d87b
📒 Files selected for processing (11)
.github/workflows/test.ymlCHANGELOG.mddocs/SPORTS_UNIFICATION.mdsrc/base_classes/baseball.pysrc/base_classes/hockey.pysrc/base_classes/sports/__init__.pysrc/base_classes/sports/core.pysrc/base_classes/sports/modes.pytest/test_sports_base_characterization.pytest/test_sports_core_promotions.pytest/test_sports_modes_promotions.py
Follow-up review findings on the promoted base-class methods. _is_game_really_over: - `period` present-but-None raised TypeError on `None >= FINAL_PERIOD`, taking down the whole live-update pass (_detect_stale_games has no try/except). Same failure shape as the null `period_text` already fixed. - An expired clock spelled "00:00" normalizes to "0000", which matched none of the hand-listed literals, so a finished game with a two-digit minute clock stayed on the scoreboard forever. Compare numerically. SportsCore: - _load_fonts kept the cwd-relative "assets/fonts/..." literals the _font_root() seam exists to remove, so every scoreboard font degraded to PIL's default face outside the install root. - _should_log read self._last_warning_time unguarded while only an unrelated method initialized it lazily; the first warning of a run raised AttributeError. Initialize it in __init__. Also documents that game_update_timestamps is written by subclasses, not by the base class, so the staleness branch is inert until B5 adoption. 14 new tests. Gates: 463 core unit, 60 plugin safety. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
|
Worked through the review. Pushed Fixed
14 new tests cover these. Gates: 463 core unit tests, 60 plugin-safety. Not changingBaseball favorites path still dereferences event-level status — not reproducible.
Wire the promoted selectors into Extract the shared selection body into
Generated by Claude Code |
Phase B2 of the sports unification. Both features exist in only some of
the nine scoreboards, so they ship as capabilities the plugin composes,
never as `if self.<feature>_enabled` branches inside the base classes: a
sport that does not opt in has none of this code in its MRO.
CelebrationMixin (afl, nrl, soccer, football)
The two lineages spelled this differently -- _check_for_goal /
celebrate_opponent_goals vs _check_for_score / celebrate_opponent_scores
-- but the bodies were identical apart from three things, each now a
seam rather than a branch:
- wording -> score_phrase() / win_phrase() hooks
- follow-up suppression -> COALESCE_SCORING_SEQUENCE, on for football
where a touchdown lands as +6 then +1, off where two increments are
two real goals
- team identity -> _favorite_key, so nrl matches on team id without
core learning why its abbreviations are ambiguous
Both config spellings are read, so a plugin adopting the mixin keeps
working with the keys already in its published schema.
Rotation strategies
The three "dialects" turned out to be one algorithm (SWRR) in two
shapes: an incremental picker holding state across calls, and a
precomputed per-cycle list. They agree within a cycle and differ only at
the boundary, so core ships both behind a name registry rather than
declaring a winner. weight_for is supplied by the host, so rotation.py
never learns what a favorite is; an unknown name degrades to "simple"
because it arrives from user config.
Each strategy is checked against a verbatim transcription of the plugin
code it replaces, over every live-game shape up to four games -- the
differential B5 will delete the bundled copies on the strength of.
185 new tests. Gates: 648 core unit, 60 plugin safety.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
ChuckBuilds
changed the title
Phases B3 and B4.
B3 -- src/common/sports_scroll.py is deliberately NOT a superset of the
ten plugin scroll_display.py copies. A method-level comparison of the
eight that share a shape (f1 and ufc are genuine forks) found a sharp
split, and the module is drawn along it:
promoted orchestration -- get_all_vegas_content_items is identical
in all eight; clear_all, get_scroll_info,
get_dynamic_duration, is_complete and display_frame are
96-100% similar
promoted settings -- one algorithm; the copies differ only in which
league keys they walk, so the ladder is data
(SCROLL_LEAGUE_KEYS) rather than a body per sport
NOT content -- prepare_scroll_content has 8 distinct bodies
across 8 plugins (145 lines, 53% similar at worst) and
_load_separator_icons 7 (6% at worst)
Same name, different job: prepare_scroll_content draws *this sport's*
game card. Merging those eight bodies would be exactly the mistake the
promotion rule exists to prevent, so the base raises NotImplementedError
rather than rendering something plausible -- a base that rendered
something would let a plugin ship a silently blank scroll.
The one behavior added over the plugin copies is native
global_config['target_fps'] support. The bundled copies hardcode ~100
FPS via scroll_delay and never consult the global target; Part A
threaded it through each copy by hand, and this makes that threading
legacy compatibility rather than the mechanism.
66 tests, including three against the real ScrollHelper rather than a
double -- a suite built entirely on MagicMock would sail straight past a
rename in the helper.
B4 -- bump src/__init__.py to 3.2.0 and close the CHANGELOG's Unreleased
section against it. This is the number the sunset rule keys on: the
first core release shipping the unified sports library, and therefore
the floor a plugin sets ledmatrix_min_version to before deleting its
bundled copies. The version bump and the changelog release heading move
together on purpose -- separating them would leave a commit whose
changelog announces 3.2.0 while the code still reports 3.1.0.
Nothing here changes what an existing plugin loads; adoption is B5.
Gates: 714 core unit, 66 plugin safety.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
|
@coderabbitai review Three commits have landed since the last pass, so re-requesting (auto-review skips this PR because its base is New since the last review:
Also addressed from your previous pass: the null- Two suggestions I did not take, with reasoning:
Generated by Claude Code |
|
I will treat the promoted selectors as intentionally inert until phase B5. I will also preserve the byte-comparison requirement for the promoted selector bodies during this phase. ✏️ Learnings added
✅ Action performedReview finished.
|
Follow-ups from the second review pass, both on already-fixed findings: _should_log accepted a warning_type and ignored it, sharing one timestamp across every kind of warning -- so an API-error warning silenced an unrelated cache warning for the next minute, and whichever fired first won. Cooldowns are now keyed by type. Nothing in core calls this method, so no behavior regressed; _last_warning_time is kept in step for subclasses that read it directly. _load_fonts logged through the module-level logger, dropping the manager context, and had no return type hint. It now uses self.logger (set well before _load_fonts runs) and names the directory it searched -- the bare "Fonts not found" sent people hunting for a font-format problem when the actual cause is an install missing assets/fonts. Gates: 717 core unit, 66 plugin safety. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
B5 cannot ship until this PR merges and 3.2.0 exists -- a plugin cannot floor ledmatrix_min_version at a release that does not exist, and an unguarded src.common.sports_scroll import would break every user on 3.1.0. The pilot has been validated ahead of that gate: hockey's scroll_display.py adopted against a core carrying 3.2.0 goes from 691 to 289 lines with all 16 harness renders byte-for-byte identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/base_classes/sports/capabilities/rotation.py (3)
200-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the
factoryargument.
register_rotation_strategychecks the name but not the type. A caller that passes an instance, or a class that is not aRotationStrategysubclass, registers successfully. The failure then appears later insideget_rotation_strategyor at the firstschedule()call, far from the cause.🛡️ Proposed guard
if not name: raise ValueError("rotation strategy name must be a non-empty string") + if not (isinstance(factory, type) and issubclass(factory, RotationStrategy)): + raise TypeError( + f"rotation strategy {name!r} must be a RotationStrategy subclass, " + f"got {factory!r}" + ) factory.name = nameAs per coding guidelines: "Validate inputs and handle errors early (Fail Fast principle)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/capabilities/rotation.py` around lines 200 - 210, Update register_rotation_strategy to validate that factory is a class and a subclass of RotationStrategy before assigning factory.name or modifying _REGISTRY; raise an appropriate validation error for invalid instances or unrelated classes while preserving the existing name validation and registration behavior.Source: Coding guidelines
111-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider an upper bound on weights.
weights()clamps only the lower bound.schedule()then loopstotal_weighttimes with an inner pass over every game. A hostweight_forthat returns a large value, for example from a misread config field, produces a very long loop on the display thread. On a Raspberry Pi this stalls rendering.Clamp the weight to a documented maximum in
RotationStrategy.weights, for the same reason the lower bound is clamped.🛡️ Proposed guard
class RotationStrategy: + #: Upper bound on a per-game weight. A cycle is `sum(weights)` long, so an + #: unbounded weight would make `schedule` loop for an unbounded time on the + #: display thread. + MAX_WEIGHT = 16 +- weights[gid] = max(1, weight) + weights[gid] = min(self.MAX_WEIGHT, max(1, weight))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/capabilities/rotation.py` around lines 111 - 129, Update RotationStrategy.weights to clamp each computed game weight to a documented maximum as well as the existing lower bound, before schedule consumes the weights. Define or reuse a clear maximum-weight constant near the existing bounds, preserving normal weighting behavior while preventing schedule’s total_weight loop from becoming excessively large.Source: Coding guidelines
183-194: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBuild the preview from
type(self).
schedule()hardcodesSmoothWeightedRotationfor the preview. A plugin that subclasses this strategy and overridesnext_gamegets a preview of the base algorithm, not of its own ordering. The returned order then does not match what repeatednext_gamecalls produce, which is exactly what the docstring promises.
schedule()also callsweight_foron every game for each of thesum(weights)preview steps. If a hostweight_fordoes real work, this multiplies that cost.♻️ Proposed change
- preview = SmoothWeightedRotation(self._weight_for) + preview = type(self)(self._weight_for) preview._current = dict(self._current)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/capabilities/rotation.py` around lines 183 - 194, Update schedule() to construct the preview using type(self), preserving subclass implementations of next_game and matching repeated calls on the strategy. Reuse the weights already computed for the schedule when configuring the preview so _weight_for is not recalculated for every game at every preview step, while preserving the existing ordering and empty-result behavior.src/base_classes/sports/capabilities/celebrations.py (2)
361-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
has_active_celebration()and drop the celebration after a render failure.Line 368 repeats the window check that
has_active_celebration()already implements at Line 125. The two can drift.A render failure also keeps
active_celebrationarmed. The next frame retries the same broken render and logs the same error again, for the whole celebration window. Clear the celebration after a failure so the scorebug resumes immediately.♻️ Proposed change
celebration = self.active_celebration if celebration: - if time.time() - celebration["started_at"] < self.celebration_duration: + if self.has_active_celebration(): try: self._draw_celebration_layout(celebration, force_clear) return True except Exception as e: self.logger.error(f"Error drawing celebration: {e}", exc_info=True) - else: - self.active_celebration = None - # Reset the dwell so the scorebug resumes on the scoring/winning - # game for a full duration before rotation can move on. - self.last_game_switch = time.time() + self.active_celebration = None + self.last_game_switch = time.time() + else: + self.active_celebration = None + # Reset the dwell so the scorebug resumes on the scoring/winning + # game for a full duration before rotation can move on. + self.last_game_switch = time.time() return super().display(force_clear)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/capabilities/celebrations.py` around lines 361 - 379, Update display() to use has_active_celebration() for the active-window check instead of duplicating the timestamp comparison. In the _draw_celebration_layout() exception handler, clear active_celebration before logging and falling back to the normal scorebug so subsequent frames do not retry the failed render.
145-151: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrune
_score_baselinesfor games that leave the live set.
_score_baselinesgains an entry per game id. Only_check_for_winremoves an entry. A game that disappears from the live list without a final update keeps its entry forever. On a long-running Raspberry Pi board the dict grows across every day of the season.Add a prune step that keeps only ids present in the current live set, similar to the state pruning in
SmoothWeightedRotation.next_game.As per coding guidelines: "Clean up resources regularly to manage memory effectively" and "Optimize code for Raspberry Pi's limited RAM and CPU capabilities".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/capabilities/celebrations.py` around lines 145 - 151, Prune stale entries from _score_baselines during the live-game update flow, retaining only game IDs present in the current live set before or alongside baseline refreshes. Follow the existing pruning approach used by SmoothWeightedRotation.next_game, while preserving _check_for_win behavior and first-sighting baseline initialization.Source: Coding guidelines
test/test_sports_scroll.py (2)
178-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference
DEFAULT_SCROLL_SETTINGSinstead of the literal50.0.The other tests in this class read the expected value from
DEFAULT_SCROLL_SETTINGS. Line 181 duplicates the default speed as a literal. If the default changes, this test fails for a reason unrelated to null-block tolerance, which is what the test targets.♻️ Proposed change
def test_a_null_league_block_is_tolerated(self, build): """`config['nhl'] = None` appears in hand-edited configs.""" display = build({"nhl": None}) - assert display._get_scroll_settings()["scroll_speed"] == 50.0 + assert (display._get_scroll_settings()["scroll_speed"] + == DEFAULT_SCROLL_SETTINGS["scroll_speed"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_sports_scroll.py` around lines 178 - 181, Update test_a_null_league_block_is_tolerated to compare scroll_speed against DEFAULT_SCROLL_SETTINGS instead of the literal 50.0, while preserving the test’s focus on tolerating a null NHL configuration block.
513-530: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the timeout failure distinguishable, and reconsider the wall-clock budget for the Raspberry Pi.
The loop needs roughly 600 frames to clear a 300px strip at 0.5 px/frame, and each frame is gated on
scroll_delayof 0.001 s. On a Raspberry Pi, the per-frame PIL crop and display push cost much more than on CI. If the target hardware cannot sustain that rate, the loop exits on the 20 s deadline and line 529 fails with no indication that a timeout caused it.Assert the timeout explicitly so a slow run reports a timeout instead of a scroll defect.
♻️ Proposed change
deadline = time.time() + 20 while not real.is_complete() and time.time() < deadline: real.display_frame() time.sleep(0.0012) - assert real.is_complete() is True + assert time.time() < deadline, ( + "scroll did not finish inside the 20s budget; the host may be too " + "slow to sustain the configured frame rate") + assert real.is_complete() is True assert display.scroll_helper.scroll_position > 128As per coding guidelines: "Ensure tests are compatible with Raspberry Pi environment".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_sports_scroll.py` around lines 513 - 530, Update test_a_strip_is_built_and_scrolls_to_completion to track whether the deadline expires before scrolling completes, and assert that the loop did not time out before asserting completion. Use a distinct timeout assertion message so Raspberry Pi performance failures are clearly separated from scroll behavior failures, while preserving the existing scrolling and deadline logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/SPORTS_UNIFICATION.md`:
- Around line 169-171: Correct the malformed custom-strategy sentence in
docs/SPORTS_UNIFICATION.md lines 169-171 to use “A plugin that needs an ordering
core does not ship calls register_rotation_strategy instead of core growing a
branch.” Update the corresponding register_rotation_strategy docstring in
src/base_classes/sports/capabilities/rotation.py lines 200-210 to say “A plugin
that needs an ordering core does not ship registers it here, rather than core
growing a sport-specific branch.”
In `@src/base_classes/sports/capabilities/celebrations.py`:
- Around line 257-260: Update the celebration capability log messages in the
armed log and the corresponding logs near lines 326 and 373 to include a
consistent structured source prefix identifying the celebration capability,
following the project’s bracketed context format while preserving the existing
message content.
- Around line 50-66: Validate and normalize celebration_duration during __init__
instead of storing the raw mode_config value: coerce numeric input safely and
clamp it to a positive, sensible maximum range so later comparisons and
rendering cannot receive strings, zero, or negative values. Keep the existing
default behavior and use the normalized value assigned by the Celebrations
initializer.
In `@src/common/sports_scroll.py`:
- Around line 271-296: Extend the try/except in display_scroll_frame to include
scroll_helper.update_scroll_position() and scroll_helper.get_visible_portion(),
while preserving the existing early returns for missing cached or visible
content and the current exception logging and False return behavior.
- Around line 399-413: Wrap the prepare_scroll_content call in
prepare_and_display with exception handling so subclass failures for one game
type degrade gracefully instead of escaping the shared orchestration entry
point. Log the exception using the existing project logging approach, return
False on failure, and only update _current_game_type when preparation succeeds.
- Around line 217-265: Update _configure_scroll_helper to defensively coerce
scroll_speed, scroll_delay, and dynamic_duration from settings, handling None,
malformed, and non-numeric values without raising during initialization. Reuse
the existing safe fallback pattern from _resolve_target_fps, preserving valid
configured values and applying defaults before arithmetic or helper
configuration.
In `@test/test_sports_scroll.py`:
- Around line 340-348: Align the empty game-type sentinel used by
SportsScrollDisplay.clear() and SportsScrollDisplayManager.clear_all() so both
represent no active type consistently, preserving the manager’s game_type or
self._current_game_type selection behavior. Update the related clear-state
assertions, including test_clear_drops_everything, to verify the chosen sentinel
and prevent mismatches between manager and display state.
---
Nitpick comments:
In `@src/base_classes/sports/capabilities/celebrations.py`:
- Around line 361-379: Update display() to use has_active_celebration() for the
active-window check instead of duplicating the timestamp comparison. In the
_draw_celebration_layout() exception handler, clear active_celebration before
logging and falling back to the normal scorebug so subsequent frames do not
retry the failed render.
- Around line 145-151: Prune stale entries from _score_baselines during the
live-game update flow, retaining only game IDs present in the current live set
before or alongside baseline refreshes. Follow the existing pruning approach
used by SmoothWeightedRotation.next_game, while preserving _check_for_win
behavior and first-sighting baseline initialization.
In `@src/base_classes/sports/capabilities/rotation.py`:
- Around line 200-210: Update register_rotation_strategy to validate that
factory is a class and a subclass of RotationStrategy before assigning
factory.name or modifying _REGISTRY; raise an appropriate validation error for
invalid instances or unrelated classes while preserving the existing name
validation and registration behavior.
- Around line 111-129: Update RotationStrategy.weights to clamp each computed
game weight to a documented maximum as well as the existing lower bound, before
schedule consumes the weights. Define or reuse a clear maximum-weight constant
near the existing bounds, preserving normal weighting behavior while preventing
schedule’s total_weight loop from becoming excessively large.
- Around line 183-194: Update schedule() to construct the preview using
type(self), preserving subclass implementations of next_game and matching
repeated calls on the strategy. Reuse the weights already computed for the
schedule when configuring the preview so _weight_for is not recalculated for
every game at every preview step, while preserving the existing ordering and
empty-result behavior.
In `@test/test_sports_scroll.py`:
- Around line 178-181: Update test_a_null_league_block_is_tolerated to compare
scroll_speed against DEFAULT_SCROLL_SETTINGS instead of the literal 50.0, while
preserving the test’s focus on tolerating a null NHL configuration block.
- Around line 513-530: Update test_a_strip_is_built_and_scrolls_to_completion to
track whether the deadline expires before scrolling completes, and assert that
the loop did not time out before asserting completion. Use a distinct timeout
assertion message so Raspberry Pi performance failures are clearly separated
from scroll behavior failures, while preserving the existing scrolling and
deadline logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5249f022-3aed-4e18-a085-c256b9cf0ab8
📒 Files selected for processing (14)
.github/workflows/test.ymlCHANGELOG.mddocs/SPORTS_UNIFICATION.mdsrc/__init__.pysrc/base_classes/sports/capabilities/__init__.pysrc/base_classes/sports/capabilities/celebrations.pysrc/base_classes/sports/capabilities/rotation.pysrc/base_classes/sports/core.pysrc/base_classes/sports/modes.pysrc/common/sports_scroll.pytest/test_sports_capabilities.pytest/test_sports_core_promotions.pytest/test_sports_modes_promotions.pytest/test_sports_scroll.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/test.yml
- test/test_sports_modes_promotions.py
- src/base_classes/sports/modes.py
- src/base_classes/sports/core.py
…classes Review pass on the phase B2-B4 changes. Every fix here is the same shape as the crashes this PR already fixed in the hockey and baseball extractors: a config or feed value that is present-but-wrong reaching arithmetic or a comparison on a path with no guard. celebrations: - celebration_duration is coerced and floored at init. It is compared numerically in display() *outside* any try block, so a string from a hand-edited config propagated a TypeError straight out; zero or negative armed a celebration that could never render. - A render failure now disarms instead of staying armed. It previously retried the same broken render on every frame for the rest of the window -- a traceback per frame, and no scorebug either. - prune_score_baselines() for the live set. Only _check_for_win removed entries, so a game that left the live list any other way leaked its baseline and the dict grew all season. - display() reuses has_active_celebration() rather than repeating its window comparison, and log lines carry a [Celebrations] prefix. rotation: - MAX_WEIGHT ceiling. A cycle is sum(weights) long and each step scans every game, so an unbounded weight from a misread config spins the display thread -- on a Pi that stalls rendering outright. - register_rotation_strategy rejects a non-subclass factory at registration instead of failing frames later inside schedule(). - schedule() previews through type(self), so a subclass overriding next_game is previewed with its own ordering -- which is what the method promises. sports_scroll: - scroll_speed / scroll_delay coerced. dict.get(key, default) only helps when the key is absent; present-but-null reached the multiplication inside __init__ and the display failed to construct at all. - update_scroll_position and get_visible_portion moved inside the try. They ran outside it, so a raise there reached the plugin's frame loop despite the comment promising none can. - prepare_and_display guards the subclass call, so one sport's bad payload cannot take down the shared orchestration for the others. - _current_game_type spells "nothing active" as "" in both classes; the manager said None while the display said "". Not taken: the report that baseball's favorite-team debug path still reads event-level status. Verified against current code -- there are no remaining game_event["status"] reads in that file; it was fixed in 2486bdb and the finding is stale. Gates: 747 core unit, 66 plugin safety. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
|
Addressed in Taken (all of them bar one):
Not taken — one, and I think it's stale:
Verified against current code: New coverage for all of the above: 747 core unit tests and 66 plugin-safety tests pass. Generated by Claude Code |
The competition-level status fallback fixed the inning lookup, but the favourite-team debug block a few lines above still read the event top-level game_event["status"]. MiLB events (synthesized from the MLB Stats API into an ESPN-like shape) populate only the competition-level status, so the identical event that extracted fine for a non-favourite raised KeyError and returned None once the team was a favourite. Worst possible shape for the bug: it only hit the games the user cared most about, and only on the path meant to help diagnose them. The existing regression test missed it because it never passes favourites, so is_favorite_game was False and the block never ran. Uses the validated competition-level `status`, which _extract_game_details_common guarantees is present by that point. Adds a favourites-passing companion test; confirmed it reproduces the KeyError without the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Addresses the last remaining sub-point on the modes.py review thread. The design finding itself is already handled: the base class documents that it only reads game_update_timestamps and that a subclass's update() owns writing "last_seen" (and afl/etc. do, so stale-game eviction works in practice). The one concrete gap was the missing annotation -- _zero_clock_timestamps is typed Dict[str, float] while this nested map had none. Now Dict[str, Dict[str, float]]. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Two Minor findings from CodeRabbit's first review of this PR. - resolve_font_path: reject relative font names carrying path components. font_name comes from plugin config, which the web UI writes; a value like "../../config/config.json" escaped assets/fonts/ after os.path.join and let a config probe arbitrary paths for existence (disclosure unlikely, since Pillow/freetype reject non-font files, but the probe is real). Relative names must now be bare filenames (os.path.basename(name) == name); absolute paths keep their existing isfile() gate. Test confirms the traversal resolved the real config.json before the guard. - build_manager fixture: patch requests.Session.get BEFORE constructing the manager. Construction creates both SportsCore.session and the ESPNDataSource.session; the old code only replaced manager.session after the fact, leaving data_source.session real and able to reach the network on an accidental fetch. Patching the class makes every session built in the fixture offline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…s-unification-phase1
#425 was squash-merged, so #426's base content now lives in main under a different SHA. Reconciled the three resulting conflicts: - .github/workflows/test.yml: kept #426's superset test list (its unification suites plus test_element_style.py); main had only element_style from #425. - CHANGELOG.md: kept #426's version, which promotes main's "Unreleased" section into the "3.2.0" release and is a verified superset of main's content (no lines dropped). - test_sports_base_characterization.py: kept #426's side for all three blocks. Two are semantic, not cosmetic: #425 pinned the pre-fix behaviour (hockey/baseball events dropped -> *_returns_none), while #426 carries the actual fixes (2486bdb, 2eea7a7) and updated the tests to *_still_extracts. Taking main's side would fail against #426's fixed code. The third is the get_background_service import path, which is .core in #426 after the package split. Also refreshed a stale comment that still named the removed test. #424's global_config plumbing merges in cleanly; verified the interlock end to end on the merged tree (global_config target_fps -> SportsScrollDisplay 90.0). 435 tests pass across the affected suites; the lone failure (test_get_system_status) is one of the four pre-existing on main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/sports_scroll.py (1)
191-199: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate scroll-setting blocks before reading or merging them.
If a hand-edited config sets a league block to a scalar,
(self.config.get(key) or {}).get(...)raisesAttributeError. Ifscroll_settingsis a scalar or list,{**settings, **override}raisesTypeError. Both failures occur during display construction before_coerce_float()can apply a fallback.Require both the league block and its
scroll_settingsvalue to be mappings. Skip invalid blocks with a warning, then continue to the next candidate or defaults. Apply the same validation toSCROLL_CONFIG_KEY. Add regression tests for scalar league blocks and scalarscroll_settingsvalues.As per coding guidelines, “Validate inputs and handle errors early (Fail Fast principle)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/sports_scroll.py` around lines 191 - 199, Update the scroll-settings resolution logic around the candidate loop and SCROLL_CONFIG_KEY fallback to validate that each league/default block and its scroll_settings value are mappings before accessing or merging them. For invalid scalar or list values, emit a warning and continue to the next candidate or defaults so _coerce_float() can still apply fallbacks; add regression tests covering scalar league blocks and scalar scroll_settings values.Source: Coding guidelines
♻️ Duplicate comments (1)
src/base_classes/sports/capabilities/celebrations.py (1)
58-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject non-finite and excessive celebration durations.
float("inf")passes this validation.has_active_celebration()then remains true indefinitely, so the celebration can permanently take over the display. Add a finite-value check and an upper duration limit before assigningself.celebration_duration.This repeats the earlier configuration-range finding. It remains unresolved for non-finite and very large values.
As per coding guidelines: “Validate required configuration fields on initialization.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base_classes/sports/capabilities/celebrations.py` around lines 58 - 67, Update the celebration duration validation in the initialization flow around raw_duration and self.celebration_duration to reject non-finite values and durations exceeding the configured maximum before assignment. Preserve the existing minimum-duration validation, warning behavior, and 8.0-second fallback for invalid inputs, using the appropriate finite-value check and upper-bound constant or value already established by the class.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/base_classes/sports/capabilities/celebrations.py`:
- Around line 400-403: Update the active-celebration branch in the display
method to call display_manager.clear() before _draw_celebration_layout(),
including when force_clear is False, and preserve the existing update_display()
call after rendering.
In `@test/test_sports_capabilities.py`:
- Around line 509-513: Update
TestDisplayTakeover.test_expired_celebration_clears_and_defers and
test_expiry_resets_the_dwell_clock to explicitly backdate
active_celebration["started_at"] beyond the normalized celebration duration
before invoking display(). Keep the zero-duration setup and existing expiration
assertions unchanged.
---
Outside diff comments:
In `@src/common/sports_scroll.py`:
- Around line 191-199: Update the scroll-settings resolution logic around the
candidate loop and SCROLL_CONFIG_KEY fallback to validate that each
league/default block and its scroll_settings value are mappings before accessing
or merging them. For invalid scalar or list values, emit a warning and continue
to the next candidate or defaults so _coerce_float() can still apply fallbacks;
add regression tests covering scalar league blocks and scalar scroll_settings
values.
---
Duplicate comments:
In `@src/base_classes/sports/capabilities/celebrations.py`:
- Around line 58-67: Update the celebration duration validation in the
initialization flow around raw_duration and self.celebration_duration to reject
non-finite values and durations exceeding the configured maximum before
assignment. Preserve the existing minimum-duration validation, warning behavior,
and 8.0-second fallback for invalid inputs, using the appropriate finite-value
check and upper-bound constant or value already established by the class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 91697c97-6501-47fd-adfd-e0e56e9af5c1
📒 Files selected for processing (11)
docs/SPORTS_UNIFICATION.mdsrc/base_classes/baseball.pysrc/base_classes/sports/capabilities/celebrations.pysrc/base_classes/sports/capabilities/rotation.pysrc/base_classes/sports/core.pysrc/base_classes/sports/modes.pysrc/common/sports_scroll.pytest/test_sports_base_characterization.pytest/test_sports_capabilities.pytest/test_sports_core_promotions.pytest/test_sports_scroll.py
🚧 Files skipped from review as they are similar to previous changes (6)
- test/test_sports_base_characterization.py
- src/base_classes/baseball.py
- src/base_classes/sports/capabilities/rotation.py
- test/test_sports_core_promotions.py
- src/base_classes/sports/modes.py
- src/base_classes/sports/core.py
CodeRabbit (Major) on the merge re-review: celebration_duration is clamped to a 1.0s floor, so the two expiry tests that configured 0 and expected instant expiration never actually hit the expiry branch. They passed only because _draw_celebration_layout raises in the harness (no real fonts) and its exception branch clears the celebration the same way -- so they were really re-testing the render-failure path, not expiry. Now use a valid 1s duration, backdate started_at past the window, and mock _draw_celebration_layout with assert_not_called() so an expired celebration provably does NOT render. Verified discriminating: both fail if has_active_celebration is forced to never expire. Production code unchanged -- the expiry logic was already correct; only the tests were mismodelling it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
2 participants
Pull Request
Summary
Phases B1 and B2 of the shared-sports-code unification, on top of the phase-0 safety net in #425 (this PR is stacked on it — retarget to
mainonce #425 merges).The goal is to let the nine sports scoreboards converge onto shared core code without creating a 2,500-line god class that all nine inherit.
docs/SPORTS_UNIFICATION.mdis the architecture doc; the short version is that upgradability, reusability and modularity are three separate problems and get three separate mechanisms:hasattrcapability probing, a frozen view-model contract, and a sunset rule keyed onCHANGELOG.md.if self.<feature>_enabledbranches inside a base class. Hockey has no celebrations, so the celebration code is not in hockey's MRO at all.Type of change
Related
Stacked on #425. Refs ledmatrix-plugins#239 and closes the core half of ledmatrix-plugins#240.
What's in here
B1 — package split + promotions
sports.py→src/base_classes/sports/(core.py+modes.py), a pure move. The import path is unchanged:from src.base_classes.sports import SportsCorestill works via the package__init__.Nine universal methods promoted from the plugins' bundled copies — the ones present in all nine, which is the only promotion criterion. Plus the override points
_favorite_key,_config_schema_path,_font_rootand the class attributesFINAL_PERIOD/CLOCK_COUNTS_DOWN.These are deliberately inert until B5: nothing in core calls them yet, and the bodies are kept byte-comparable to the nine plugin copies, because that back-comparison is how B5 verifies each plugin can delete its copy without drift.
B2 — opt-in capabilities
CelebrationMixin(afl, nrl, soccer, football). The two lineages spelled this differently (_check_for_goal/celebrate_opponent_goalsvs_check_for_score/celebrate_opponent_scores) but the bodies were identical apart from three things, each now a seam rather than a branch: wording (score_phrase()), follow-up suppression (COALESCE_SCORING_SEQUENCE— a football touchdown lands as +6 then +1, whereas two soccer increments are two real goals), and team identity (_favorite_key, so NRL matches on team id without core learning that its abbreviations are ambiguous). Both config spellings are read, so adopting the mixin doesn't reset users' existing settings.swrr/weighted/simple, plusregister_rotation_strategy) rather than declaring a winner.weight_foris supplied by the host, sorotation.pynever learns what a favorite is.Bugs found and fixed along the way
The characterization suite in #425 pinned two extractor behaviors as-is and flagged them; this PR changes them knowingly:
statisticsarray — an unguarded subscript raisedKeyErrorinside a generator, discarding the whole event despite valid scores and status. Shot counts now fall back to0.KeyError.Plus, from review of the promoted code:
"0:00"and treated the game as over onceperiod >= 4— but baseball has no game clock andperiodis the inning, so live MLB games vanished from the 5th inning onward.periodno longer crashes the live-update pass (None >= FINAL_PERIODis aTypeError, and_detect_stale_gameshas notry/except)."00:00"now ends the game — it normalized to"0000", which matched none of the hand-listed literals, so such a game stayed on screen indefinitely. The comparison is now numeric._load_fontsno longer degrades every scoreboard font to PIL's default face when the process starts outside the install root, and_should_logno longer raisesAttributeErroron the first warning of a run.Test plan
pytest)648 core unit tests and 60 plugin-safety tests pass. The suites added here:
test_sports_core_promotions.py/test_sports_modes_promotions.py— the promoted surface, including regression tests that assert the resolved install root rather than aparents[]index (a pure-move refactor is not semantically pure when the code measures its own location — that one bit me and the tests exist so it can't again).test_sports_capabilities.py— 185 tests. Two things worth calling out:CelebrationMixinmust have none of its attributes, andSportsLive's own source must contain no celebration hooks. That's the property the design exists to buy, so it's a test rather than a convention.Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changeddocs/SPORTS_UNIFICATION.mdcarries the architecture, the override-point contract table, why each seam is a seam rather than a branch, and the rules for contributors ("never add a sport name to core"; "a capability not opted into must not execute"; "touch the view-model keys only additively").CHANGELOG.mdrecords the new modules against the version the sunset rule keys on.Plugin compatibility
Nothing in this PR changes what plugins load today — the promoted methods are inert, the capabilities are new files nothing imports yet, and the plugins' bundled copies still shadow core. Adoption happens in B5, one plugin per lineage first.
Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.md(No new config keys.
rotation_strategyand the celebration keys are read from existing plugin schemas.)Notes for reviewer
Scope note: this PR was opened as phase 1 and now also carries phase 2. B2 is purely additive — new files under
capabilities/that nothing imports yet — so it adds surface without adding risk to what already runs, and stacking it as a fourth PR behind two unmerged ones seemed worse for review than one coherent unit. Happy to split it back out if you'd rather review them separately.The two design decisions most worth your attention:
update()now, but nothing floors on them yet and nine bundled copies still shadow them, so activating early would change core behavior with no plugin exercising the new path. B5 pilots hockey/soccer/football against them first.🤖 Generated with Claude Code
https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
Summary by CodeRabbit
New Features
Bug Fixes
Documentation