fix(sports): stop an empty mode holding a blank panel for its duration - #263
Conversation
Reported for hockey: NHL recent and live sit on a blank screen for the
whole display duration. Reproduced with the plugin's real August data,
where ESPN returns no recent or live NHL games while upcoming still has
the preseason schedule -- which is why only those two modes show it.
The display controller skips a mode whose display() returns False. Both
plugins ship a bundled sports.py that is a stale fork of the core's, and
its three display() methods had lost their return values; the base was
even annotated `-> None`. Every path returns None, and the manager's
dispatcher treats a non-boolean as success ("Result is None or other -
assume success"), so an empty mode reported content and was never
skipped. Its no-games branch calls display_manager.clear(), so what it
reported as content was a blank panel, held until the duration expired.
Return True when a game was drawn and False when there was nothing to
show. That is what the dispatcher already documents ("Manager returns
True if it has content to show, False if no content"), what the core's
SportsCore/SportsUpcoming/SportsRecent do, and what the six other sports
plugins do -- hockey and lacrosse are the only two that had drifted.
Verified both directions against the real plugin with no games (display()
now returns False and draws nothing, so the mode is skipped) and with a
game (returns True and draws it). The added tests exercise the methods
against a stand-in self, needing no display, network or cache; they fail
against the unfixed sports.py for all three classes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
📝 WalkthroughWalkthroughThe Hockey Scoreboard and Lacrosse Scoreboard display methods now return boolean render-status values. New standalone tests cover empty, disabled, populated, and error states. Plugin manifests and catalog entries are updated to version 1.9.1. ChangesScoreboard display signaling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 | 30 |
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plugins/hockey-scoreboard/sports.py`:
- Around line 240-250: Ensure display() reports rendering failures as False
instead of returning True after exceptions are swallowed by
_draw_scorebug_layout() helpers. Apply the fix at
plugins/hockey-scoreboard/sports.py lines 240-250, 1553-1563, and 2055-2065, and
plugins/lacrosse-scoreboard/sports.py lines 241-251, 1554-1564, and 2055-2064:
either have each draw helper return an explicit success status that display()
propagates, or allow rendering exceptions to reach display() before its success
return; preserve existing error logging and successful True results.
In `@plugins/hockey-scoreboard/test_empty_mode_signals_no_content.py`:
- Around line 97-125: Wrap the module-scope harness in both
plugins/hockey-scoreboard/test_empty_mode_signals_no_content.py lines 97-125 and
plugins/lacrosse-scoreboard/test_empty_mode_signals_no_content.py lines 97-125
in a callable entry point, then invoke it only under an if __name__ ==
"__main__" guard. Keep the existing print, check, and sys.exit behavior
unchanged when either file is executed directly.
🪄 Autofix
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: 9bb7bb2a-007c-468c-8084-ec3b928f034b
📒 Files selected for processing (7)
plugins.jsonplugins/hockey-scoreboard/manifest.jsonplugins/hockey-scoreboard/sports.pyplugins/hockey-scoreboard/test_empty_mode_signals_no_content.pyplugins/lacrosse-scoreboard/manifest.jsonplugins/lacrosse-scoreboard/sports.pyplugins/lacrosse-scoreboard/test_empty_mode_signals_no_content.py
| try: | ||
| self._draw_scorebug_layout(self.current_game, force_clear) | ||
| # display_manager.update_display() should be called within subclass draw methods | ||
| # or after calling display() in the main loop. Let's keep it out of the base display. | ||
| return True | ||
| except Exception as e: | ||
| self.logger.error( | ||
| f"Error during display call in {self.__class__.__name__}: {e}", | ||
| exc_info=True, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg 'plugins/(hockey-scoreboard|lacrosse-scoreboard)/sports\.py$|plugins/.*(test|tests?|.*test).*\.py$' || true
echo
echo "== hockey sports outline =="
ast-grep outline plugins/hockey-scoreboard/sports.py --view compact || true
echo
echo "== lacrosse sports outline =="
ast-grep outline plugins/lacrosse-scoreboard/sports.py --view compact || true
echo
echo "== targeted hockey slices =="
sed -n '210,270p;1520,1580p;2025,2080p;30,80p' plugins/hockey-scoreboard/sports.py
echo
echo "== targeted lacrosse slices =="
sed -n '211,271p;1521,1581p;2025,2080p;30,80p' plugins/lacrosse-scoreboard/sports.pyRepository: ChuckBuilds/ledmatrix-plugins
Length of output: 25058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast, re, sys
for path in [
"plugins/hockey-scoreboard/sports.py",
"plugins/lacrosse-scoreboard/sports.py",
]:
print(f"\n== {path} ==")
text = open(path, encoding="utf-8").read().splitlines()
for start in [1553, 240, 2055]:
end = start + 18
print(f"\n--- lines {start}-{end} ---")
for i in range(start, min(end + 1, len(text) + 1)):
print(f"{i}: {text[i-1]}")
PY
echo
echo "== draw_scorebug_layout definitions/calls =="
rg -n "def _draw_scorebug_layout|def draw.*scorebug|_draw_scorebug_layout|update_display|try:" plugins/hockey-scoreboard/sports.py plugins/lacrosse-scoreboard/sports.py
echo
echo "== structural verifier for exception swallowing in display helpers =="
python3 - <<'PY'
import ast
paths = [
"plugins/hockey-scoreboard/sports.py",
"plugins/lacrosse-scoreboard/sports.py",
]
for path in paths:
tree = ast.parse(open(path, encoding="utf-8").read())
print(f"\n== {path} ==")
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == "_draw_scorebug_layout":
print(f"FunctionDef {node.name} lines {node.lineno}-{node.end_lineno}")
has_try = any(isinstance(sub, ast.Try) for sub in ast.walk(node))
has_exception_handlers = any(isinstance(h, ast.ExceptHandler) for h in node.body)
print(f" has_try_or_exception_handler={has_try or has_exception_handlers}")
for sub in ast.walk(node):
if isinstance(sub, ast.Try):
print(f" Try lines {sub.lineno}-{sub.end_lineno}")
for handler in sub.handlers:
print(f" handler exception types={[ast.dump(e) for e in handler.type] if handler.type else ['bare']}")
print(f" within_try_exits_return={bool(sub.body) and isinstance(sub.body[-1], ast.Return)}")
PYRepository: ChuckBuilds/ledmatrix-plugins
Length of output: 13851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
paths = [
"plugins/hockey-scoreboard/sports.py",
"plugins/lacrosse-scoreboard/sports.py",
]
for path in paths:
print(f"\n== {path} ==")
tree = ast.parse(open(path, encoding="utf-8").read())
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == "_draw_scorebug_layout":
print(f"FunctionDef {node.name} lines {node.lineno}-{node.end_lineno}")
has_try = any(isinstance(sub, ast.Try) and hasattr(sub, "body") for sub in ast.walk(node))
has_handlers = bool(node.type_comment) or any(isinstance(sub, ast.Try) and sub.handlers for sub in ast.walk(node))
print(f" has_try_or_exception_handler={has_try or has_handlers}")
for sub in ast.walk(node):
if isinstance(sub, ast.Try):
if sub.handlers:
print(f" Try line {sub.lineno} handlers={[ast.dump(h.type) if h.type else 'bare'] for h in sub.handlers}")
print(f" final_inside_try_is_return={isinstance(sub.body[-1], ast.Return) if sub.body else False}")
PY
echo
echo "== base display slices =="
sed -n '180,235p;1295,1340p;1817,1860p;13030,13055p;' plugins/hockey-scoreboard/sports.py plugins/lacrosse-scoreboard/sports.pyRepository: ChuckBuilds/ledmatrix-plugins
Length of output: 7828
Return False when drawing fails.
Each display() path calls _draw_scorebug_layout() and then returns True, but the render helpers catch display exceptions and continue normally. Return an explicit render status from the draw helpers, or let rendering failures propagate to display() before returning True.
📍 Affects 2 files
plugins/hockey-scoreboard/sports.py#L240-L250(this comment)plugins/hockey-scoreboard/sports.py#L1553-L1563plugins/hockey-scoreboard/sports.py#L2055-L2065plugins/lacrosse-scoreboard/sports.py#L241-L251plugins/lacrosse-scoreboard/sports.py#L1554-L1564plugins/lacrosse-scoreboard/sports.py#L2055-L2064
🤖 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 `@plugins/hockey-scoreboard/sports.py` around lines 240 - 250, Ensure display()
reports rendering failures as False instead of returning True after exceptions
are swallowed by _draw_scorebug_layout() helpers. Apply the fix at
plugins/hockey-scoreboard/sports.py lines 240-250, 1553-1563, and 2055-2065, and
plugins/lacrosse-scoreboard/sports.py lines 241-251, 1554-1564, and 2055-2064:
either have each draw helper return an explicit success status that display()
propagates, or allow rendering exceptions to reach display() before its success
return; preserve existing error logging and successful True results.
The module-scope harness ran on import and called sys.exit(), so any collector that imports test_*.py would execute it and take the process down with it. Wrap it in main() behind an __name__ guard; running the file directly is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
Thanks — took the second finding, declining the first with a reason. Harness behind Render failures reported as success — valid, but deliberately out of scope here. The premise is correct: The reason I'm not fixing it in this PR is that the core does exactly the same thing:
This PR exists specifically to end a drift between these two plugins and the core. Making them return It's also a distinctly different failure mode from the one reported. "No games" is routine (every league, every off-season); a render exception is rare and shows up as a stale or partial frame rather than a mode that never yields. And the fix you describe — threading a success status back out of the draw helpers — touches six call sites across two plugins, which is the kind of change that should land in Happy to raise it against the core as a follow-up if that's useful. |
…ss (#265) * ci: run the plugins' own tests, and repair the ones that could not pass This workflow was the safety harness plus manifest checks, so a plugin's test_*.py guarded nothing unless someone ran it by hand. That is how a display() returning None on every path survived from February to August (#263): the harness renders screens and cannot see a return value, and its fixtures seed data so an empty-state path never executes. scripts/run_plugin_tests.py already existed, with the exit-code convention that makes this gateable -- 2 for "prerequisites absent", 1 for a real failure. It was simply never wired up. Baseline across the fleet was 75 passed, 2 skipped, 9 failed, which is why: nine reds nobody could act on are not a gate, they are noise. All nine were stale tests or environment handling, not plugin bugs: baseball asserted plugin.initialized and iterated plugin.leagues, neither of which exists; its config was flat (mlb_enabled) where the plugin reads nested (config["mlb"]["enabled"]), so it ran with every league disabled and passed vacuously baseball the antialiasing test only found the core fonts when the plugin sat inside a core checkout, so it always skipped basketball imported BasketballPluginManager, a name the plugin had stopped using, from a hardcoded /home/chuck path; it now reads class_name from the manifest, which is what the loader uses, so a rename cannot pass unnoticed basketball MockLogger lacked setLevel and %-formatting, so real code under test raised inside the stub basketball its mock competitors carried a score but no team block or competitor id, which the extractor needs before it ever looks at a score -- so all five score formats reported "could not extract" for an unrelated reason hockey constructed CacheManager(config_manager=...), removed from the core's signature, and resolved config/ relative to the plugin dir; it now mirrors the pre-flight in the baseball suite and runs 29 real render cycles hockey test_recent_games.py deleted: a print-only script with zero assertions, a hardcoded /home/chuck path and a hardcoded debug date, broken since the monorepo migration lacrosse asked ESPN for the upcoming season, which is unpublished in August; it now falls back to the last completed season and skips, rather than fails, if neither has fixtures soccer asserted a bare "MCI" cache key after logo caching became size-scoped ("MCI@32x32") to stop panel sizes colliding weather almanac fonts now resolve via LEDMATRIX_CORE (so it runs instead of failing), and astral's absence is a skip, since the plugin declares it and CI installs it Now 83 passed, 2 skipped, 0 failed. The two skips are honest: one script prompts on stdin, and astral is absent locally but present in CI. The changed-plugin detection now yields two lists. Shipped-code changes still drive the version gate and harness; test-only edits no longer do, which the existing test/ exclusion had always intended but missed for the root-level test_*.py where most of these live. A second list including tests drives the unit-test run, so a PR that only edits a test still has to prove it passes. Also gitignores emulator_config.json, which RGBMatrixEmulator drops into whatever directory the suites are invoked from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * test: assert the baseball config lands, and narrow two import skips Two review findings, both about a check that could pass without meaning anything -- which is the failure this PR exists to remove. The baseball suite printed which leagues were enabled but never asserted it. That is exactly how it came to run green over a plugin with every league disabled, so a silent return to the flat-config state would have gone unnoticed again. Assert MLB is the only one enabled. The two skip guards caught ImportError broadly, so a ModuleNotFoundError raised from *inside* the core, or from inside astral, would have been filed as "not installed" and skipped. Match on exc.name and re-raise anything else: a genuine breakage should be a failure, not a skip nobody reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes the reported hockey bug: NHL recent and live sit on a blank screen for the whole display duration.
Reproduction
Reproduced with the plugin's real data. It's August, so ESPN returns no recent or live NHL games (
NHLLiveManager - Fetched 0 games for the last 2 days) while upcoming still has the preseason schedule — which is exactly why only those two modes show the symptom.Driving the real plugin with an empty game list, before the fix:
It reports success, and draws nothing.
Cause
The display controller skips a mode whose
display()returnsFalse(display_controller.py, "If display() returned False, skip to next mode immediately"). Both plugins ship a bundledsports.pythat is a stale fork of the core's, and its threedisplay()methods had lost their return values — the base was even annotated-> None:Every path returns
None, and the manager's dispatcher treats a non-boolean as success:So an empty mode reports content and is never skipped — and because the no-games branch calls
display_manager.clear(), what it reports as content is a blank panel, held until the duration expires.Scope
Only these two plugins. The core and the other six all honour the contract the dispatcher documents:
display()returnsSportsCore/SportsUpcoming/SportsRecentTrue/FalseTrue/FalseNoneSportsLivehas nodisplay()of its own and inheritsSportsCore's, which is why live is affected alongside recent.Fix
Return
Truewhen a game was drawn andFalsewhen there was nothing to show, matching the core. After:And the positive case still works — with a game present, all three modes return
Trueand draw once.Verification
test_empty_mode_signals_no_content.pyin each plugin: 21 checks covering empty, disabled and populated states, and asserting a realbool(the dispatcher branches onresult is True/result is False, so a truthy non-bool would fall straight back into the "assume success" path). The harness runs behind a__main__guard so importing it is a no-op. Confirmed these fail against the unfixedsports.pyfor all three classes.test_lacrosse_plugin.pyhits live ESPN for an out-of-season date range, andtest_core_fallback.py/test_hockey_emulator.pyskip without a core/display.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Bug Fixes
Chores