Sports unification phase 0: safety net, cwd-independent fonts, element_style - #425
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
|
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 (3)
📝 WalkthroughWalkthroughThe pull request adds schema-driven element styling, cwd-independent font loading, sports characterization tests, a dedicated core-test workflow, and changelog entries. ChangesElement styling
Sports characterization
CI and release records
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SchemaManager
participant ElementStyleResolver
participant load_font
SchemaManager->>SchemaManager: load_schema
SchemaManager->>SchemaManager: expand_style_elements
SchemaManager-->>ElementStyleResolver: expanded schema and defaults
ElementStyleResolver->>load_font: load_font(font_name, size)
load_font-->>ElementStyleResolver: cached or fallback font
ElementStyleResolver-->>SchemaManager: resolved style values
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 | 124 |
| Duplication | 0 |
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.
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
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.github/workflows/test.yml (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
RGBMatrixEmulatorin CI.
.github/workflows/test.ymlinstalls the package with an unbounded specifier in both jobs, andrequirements-emulator.txtalso allows any release. Add a tested exact version or constraints entry and remove the unbounded installs.🤖 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 @.github/workflows/test.yml around lines 52 - 56, Pin RGBMatrixEmulator to the tested exact version or shared constraints entry in both CI jobs and requirements-emulator.txt. Update the dependency installation steps and remove the unbounded RGBMatrixEmulator installs, while keeping the existing dependency installation flow intact.test/test_element_style.py (1)
236-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a fixture that clears the module-global
_font_cachebetween tests.
test_same_font_object_from_cacheasserts identity across two resolvers. The assertion depends onsrc.element_style._font_cache, which persists for the whole session. The current suite stays deterministic because every cache key uses an absolute resolved path. A future test that asserts a cache miss, or that patches font loading, would become order-dependent. Reset the global in setup.As per coding guidelines: "Clean setup and teardown between tests to reset state".
♻️ Proposed fixture
def _resolver(config, schema_path): return ElementStyleResolver(config, defaults_from_schema_file(schema_path)) + + +@pytest.fixture(autouse=True) +def _clear_font_cache(): + """Reset the module-global font cache so tests stay order-independent.""" + from src import element_style + element_style._font_cache.clear() + yield + element_style._font_cache.clear()Keep
test_same_font_object_from_cacheas is. Bothstyle()calls run inside one test, so the cache is still populated by the first call.🤖 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_element_style.py` around lines 236 - 243, Add an autouse fixture in test/test_element_style.py that clears the module-global _font_cache before each test, and optionally after each test if needed for teardown. Import or reference the cache through its existing element_style module symbol without changing test_same_font_object_from_cache, so both style() calls still share the cache within that test.Source: Coding guidelines
test/test_sports_base_characterization.py (4)
584-591: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test name with its assertions.
The name states that the call does not raise. The body also asserts that the placeholder renderer draws ink. Rename the test so the name reports both checks.
♻️ Suggested rename
- def test_draw_scorebug_layout_direct_call_does_not_raise( + def test_draw_scorebug_layout_direct_call_draws_ink( self, build_manager):As per coding guidelines: "Use descriptive test names that explain what they test".
🤖 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_base_characterization.py` around lines 584 - 591, Rename test_draw_scorebug_layout_direct_call_does_not_raise to a descriptive name that explicitly indicates both the direct call does not raise and the placeholder renderer produces visible image content; leave the test assertions and implementation unchanged.Source: Coding guidelines
354-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to the harness classes, and consider one shared
_fetch_data.
_RecentHarnessand_LiveHarnesshave no docstring, while_UpcomingHarnesshas one. The three classes also repeat the identical_fetch_databody. A small mixin removes the duplication and keeps the harness intent in one place.♻️ Suggested consolidation
-class _UpcomingHarness(Hockey, SportsUpcoming): - """Cheapest concrete SportsUpcoming: hockey extractor + cache-fed data.""" - - def _fetch_data(self): - return self.cache_manager.get(f"{self.sport_key}_schedule") - - -class _RecentHarness(Hockey, SportsRecent): - - def _fetch_data(self): - return self.cache_manager.get(f"{self.sport_key}_schedule") - - -class _LiveHarness(HockeyLive): - - def _fetch_data(self): - return self.cache_manager.get(f"{self.sport_key}_schedule") +class _CacheFedMixin: + """Replaces the network fetch with a read from the mocked cache.""" + + def _fetch_data(self): + return self.cache_manager.get(f"{self.sport_key}_schedule") + + +class _UpcomingHarness(_CacheFedMixin, Hockey, SportsUpcoming): + """Cheapest concrete SportsUpcoming: hockey extractor + cache-fed data.""" + + +class _RecentHarness(_CacheFedMixin, Hockey, SportsRecent): + """Cheapest concrete SportsRecent: hockey extractor + cache-fed data.""" + + +class _LiveHarness(_CacheFedMixin, HockeyLive): + """Cheapest concrete SportsLive: hockey live extractor + cache-fed data."""As per coding guidelines: "Use docstrings for classes and complex functions".
🤖 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_base_characterization.py` around lines 354 - 368, Add docstrings to _RecentHarness and _LiveHarness describing their test-harness roles, and consolidate the identical _fetch_data implementation into a shared mixin or base helper reused by _UpcomingHarness, _RecentHarness, and _LiveHarness. Preserve each class’s existing inheritance and cache lookup behavior.Source: Coding guidelines
138-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type hints to the module helpers.
make_probe,extract,make_event, and_competitorhave no parameter or return annotations. Annotations make the fixture shapes explicit for future maintainers of these characterization tests.♻️ Suggested annotations
-def make_probe(favorites=None): +def make_probe(favorites: list[str] | None = None) -> MagicMock: """Bare-bones SportsCore stand-in for exercising the real extractors unbound (same pattern as TestViewModelContract in test_skin_system).""" @@ -def extract(sport_cls, event, favorites=None): +def extract(sport_cls: type, event: dict, + favorites: list[str] | None = None) -> dict | None: return sport_cls._extract_game_details(make_probe(favorites), event)As per coding guidelines: "Use type hints for function parameters and return values".
🤖 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_base_characterization.py` around lines 138 - 156, Add parameter and return type annotations to the test helper functions make_probe, extract, make_event, and _competitor, using types that accurately describe their fixture inputs, generated objects, and optional favorites. Keep the helper behavior unchanged and ensure all function parameters and return values are explicitly typed.Source: Coding guidelines
508-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the two favorite-team scenarios into separate tests.
This test covers a matching favorite and a non-matching favorite. Separate tests report which scenario regressed without further inspection.
♻️ Suggested split
def test_filters_by_favorite_teams(self, build_manager): manager = build_manager(_RecentHarness, make_schedule(), show_favorite_teams_only=True, favorite_teams=["TOR"]) manager.update() assert _ids(manager.games_list) == ["9001"] + def test_unknown_favorite_team_yields_no_games(self, build_manager): stranger = build_manager(_RecentHarness, make_schedule(), show_favorite_teams_only=True, favorite_teams=["XXX"]) stranger.update() assert stranger.games_list == [] assert stranger.current_game is NoneAs per coding guidelines: "Each test should verify a single responsibility".
🤖 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_base_characterization.py` around lines 508 - 520, Split test_filters_by_favorite_teams into two tests: one verifying matching favorite teams returns game "9001", and another verifying a non-matching team produces an empty games_list and no current_game. Keep each test’s existing setup and assertions focused on its single scenario.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/element_style.py`:
- Around line 105-116: Update the relative-path handling in the font resolution
logic to accept only a bare filename, rejecting any font_name containing path
components before constructing candidates. Preserve absolute-path handling and
existing candidate lookup for valid filenames, returning None for invalid
relative values.
In `@test/test_sports_base_characterization.py`:
- Around line 423-430: Update the manager fixture around cls(...) to patch
requests.Session before constructing the manager, configuring every created
session’s get operation to raise requests.exceptions.ConnectionError; keep the
offline guard in place during construction so both SportsCore.session and
ESPNDataSource.session are covered, then return the constructed manager.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 52-56: Pin RGBMatrixEmulator to the tested exact version or shared
constraints entry in both CI jobs and requirements-emulator.txt. Update the
dependency installation steps and remove the unbounded RGBMatrixEmulator
installs, while keeping the existing dependency installation flow intact.
In `@test/test_element_style.py`:
- Around line 236-243: Add an autouse fixture in test/test_element_style.py that
clears the module-global _font_cache before each test, and optionally after each
test if needed for teardown. Import or reference the cache through its existing
element_style module symbol without changing test_same_font_object_from_cache,
so both style() calls still share the cache within that test.
In `@test/test_sports_base_characterization.py`:
- Around line 584-591: Rename
test_draw_scorebug_layout_direct_call_does_not_raise to a descriptive name that
explicitly indicates both the direct call does not raise and the placeholder
renderer produces visible image content; leave the test assertions and
implementation unchanged.
- Around line 354-368: Add docstrings to _RecentHarness and _LiveHarness
describing their test-harness roles, and consolidate the identical _fetch_data
implementation into a shared mixin or base helper reused by _UpcomingHarness,
_RecentHarness, and _LiveHarness. Preserve each class’s existing inheritance and
cache lookup behavior.
- Around line 138-156: Add parameter and return type annotations to the test
helper functions make_probe, extract, make_event, and _competitor, using types
that accurately describe their fixture inputs, generated objects, and optional
favorites. Keep the helper behavior unchanged and ensure all function parameters
and return values are explicitly typed.
- Around line 508-520: Split test_filters_by_favorite_teams into two tests: one
verifying matching favorite teams returns game "9001", and another verifying a
non-matching team produces an empty games_list and no current_game. Keep each
test’s existing setup and assertions focused on its single scenario.
🪄 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: fdd3ec9e-1e83-4e4b-8598-62aebb19b1ee
📒 Files selected for processing (7)
.github/workflows/test.ymlCHANGELOG.mdsrc/element_style.pysrc/font_manager.pysrc/plugin_system/schema_manager.pytest/test_element_style.pytest/test_sports_base_characterization.py
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
#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
Pull Request
Summary
Phase 0 of the shared-sports-code unification planned in ledmatrix-plugins#239 (see its
docs/plugin-development/08-shared-sports-code.md): build the safety net before any refactor ofsrc/base_classes/sports.py, and close two core gaps that PR surfaced — cwd-dependent font resolution and the missingsrc/element_style.pymodule three plugins already guard-import.Type of change
Related issues
Refs ledmatrix-plugins#239 and complements #424 (
global_configonBasePlugin).What's in here (5 commits)
FontManagerresolvedassets/fontsrelative to the process cwd, so any process started outside the install root (the plugin safety harness on CI being the recurring case) silently lost every font and degraded plugins to PIL's default face — three plugins grew per-plugin workarounds for exactly this. Catalog population now falls back to the install root derived from the module's own location; behavior from the install root is unchanged.CHANGELOG.mdwith the module-availability release discipline the plugins repo's bundled-copy sunset rule depends on.unit-testsjob enrolling the suites that existed but never ran in CI (skin system, font manager, data sources, extractors, scroll helper, adaptive layout, loader compat) plus the two new suites below — 338 tests.src/element_style.py— the per-element style resolverof-the-day,ledmatrix-music, andfootball-scoreboardalready import behind guarded fallbacks. Untouched configs resolve to exactly the classic values (byte-identical rendering, proven by of-the-day's committed goldens passing unchanged);schema_manager.load_schema()expandsx-style-elementsdeclarations so the config form shows the richer UI. of-the-day's previously-skipped 9-test spec suite now runs and passes.src/base_classes/sports.pybehavior (extractor key contract per sport, update() flows against cache-seeded fixtures under frozen time, rendering smoke, skin-seam guard rails) ahead of the phase-1 merge of the nine drifted plugin copies. Five surprising behaviors are pinned AS-IS and flagged in comments — notably hockey dropping events whose competitors lackstatistics, and baseball reading the event-level status for innings — so the merge changes them knowingly or not at all.Test plan
EMULATOR=true python3 run.py)scripts/dev_server.py)pytest)All 338 tests in the new unit job pass; the existing plugin-safety job's 60 tests pass;
FontManagerverified resolving real faces from a foreign cwd; the of-the-day plugin harness passes with every committed golden unchanged (classic-identity proof forelement_style); football's 27 adaptive/resolver tests and music's 38 plugin tests pass against the new module.Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changed(
CHANGELOG.mddocuments both the fix and the new module against the release discipline; the plugins repo's08-shared-sports-code.mdis the cross-repo companion.)Plugin compatibility
Existing consumers of
src.element_styleactivate their already-shipped code paths; plugins without it are untouched. The font fix only adds a fallback path.Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mdverified (the form is generated from
config_schema.json)(No new config keys —
x-style-elementsexpansion only affects plugins that already declare it.)Notes for reviewer
The characterization suite deliberately pins two behaviors that look like real bugs (hockey's
statisticsrequirement, baseball's event-level status read). Fixing them belongs to phase 1 where the plugin copies' corrected versions get merged back — this PR only makes the current behavior visible and diff-able. Phase 1 (merging the universal plugin-copy deltas intosrc/base_classes/sports.py) and phase 2 (opt-in celebrations/rotation capabilities) build on this safety net.🤖 Generated with Claude Code
https://claude.ai/code/session_01FgbA8SMutQQpXkMG8LMmC4
Generated by Claude Code
Summary by CodeRabbit