Repository navigation
Correctness fixes, shared view lifecycle, and build gates - #10
Merged
Merged
Conversation
…the engines The engines shared one defect class: a player's display name standing in where a stable identifier was needed. Names collide on NFL rosters and vary in spelling across nflverse source columns, so grouping and joining on them produced wrong numbers that read as plausible. ─── Identity ─── snap_share, waiver_wire — rolling windows grouped by `player`, merging two people who share a name into one series. Two "Josh Allen" rows, one steady at 90% snap share and one at 10%, gave a 36.7% rolling average for the player who never left 90%. Both window over the player id; snap_share keeps `pfr_player_id` through selection rather than discarding it. red_zone — compute_player_rz full-joined its receiving and rushing halves on the display name. nflverse fills receiver_player_name and rusher_player_name from separate source fields, so one GSIS id under two spellings split into two rows and rz_touches, rz_tds and td_pct each counted half the player's work — a dual-threat back being exactly what the tool exists to surface. The join keys on the id and coalesces a single display name through. data/stats — compute_weekly_stats_from_pbp merged its four stat lines on player_display_name, emitting one partial row per spelling for a player appearing in several roles in a week. The merge keys on player_id alone. injury_impact — teammates reduce with an explicit keep rather than polars' arbitrary default, so a teammate whose listed position varies across weeks resolves to a defined row. The searched player's team-season is chosen by sorting rather than by iteration order over `.unique()`, which polars does not guarantee; the same input had been yielding a different reported season run to run. ─── Empty results ─── start_sit, trade_value — the guard paths returned a bare pl.DataFrame() carrying no columns, which raises ColumnNotFoundError the moment a caller filters it. Each engine declares its output schema once at module level and builds both the populated and the empty result against it, so the two cannot drift apart. ─── Scoring ─── trade_value — production multiplied season PPG by a games-remaining term constant across players, so it cancelled exactly in the max-normalisation that follows and could not influence any output. Games remaining is per player: regular-season length less the current week, less one more when the team's bye is still ahead. The standalone bye component folds into it and its weight redistributes, leaving production 0.50, schedule 0.20, usage 0.20, health 0.10 — the bye counted once. trade_value — the health denominator scaled with how many seasons carried an injury report for a player, so two absences spread across two reported seasons scored differently from the same two inside one. Every player is measured against the seasons in the analysed window. data/stats — lost fumbles aggregated from a frame already filtered to pass and run plays, so a fumble on a kickoff or punt return never reached the -2 that the published nflverse asset this function substitutes for does charge. Fumbles read the unfiltered play-by-play. data/stats — the derived frame carried a position column only when rosters were supplied, while the loader reaches this path with rosters=None by design after a roster fetch failure. start_sit and trade_value both filter on that column, so the degraded path raised rather than degrading. The column is always present, null-filled when rosters are absent. red_zone — both results close on a total sort, so rows tying on the primary key hold a reproducible order. ─── Tests ─── conftest carries a four-team, four-position league across two full 18-week seasons, built so expected values are whole numbers a reader can verify by hand, alongside colliding-name, injury, schedule and play-by-play frames. Season 2025 is pristine; 2024 holds one deliberate absence for injury_impact. New suites cover trade_value, start_sit, injury_impact, red_zone and the play-by-play derivation, none of which carried tests. snap_share and waiver_wire gain the colliding-name case. Assertions are on computed values rather than frame shapes, and thresholds are exercised at their boundaries. Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
…he build
Six tool views each carried their own copy of the same load lifecycle. `activate`,
`_on_data_error`, `on_button_pressed` and `on_select_changed` appeared in all six,
`_populate_filters`, `_apply_filters` and `_fetch_data` in five, and the filter-bar
CSS block in all six; `_on_data_error` was byte-identical everywhere. Duplication
of a decision is duplication of the chance to get it wrong, and three of the six
Refresh buttons did.
─── ToolView ───
src/ffb/ui/base.py holds the lifecycle. A subclass sets ID_PREFIX and LOAD_LABEL
and implements compose_content, fetch and apply; the base owns the thread worker,
the loading pane, error reporting, refresh, filter reading and table rendering.
Refresh is the reason the seam exists. `on_button_pressed` matches the view's
refresh button and always calls the fetch with force_refresh=True. waiver_wire,
start_sit and trade_value had instead reset an activation flag and re-entered
`activate()`, reaching the fetch with force_refresh defaulting to False — so the
button re-read the same cache and reported success, against a README promising a
fresh download. Routing every view through one handler makes the divergence
unrepresentable.
`fill_table` renders an empty frame as a stated message rather than a bare header
over blank space, and reads emptiness from height so a frame carrying no columns at
all cannot raise. All six tools had rendered an empty result identically to a
loading one.
`selected` and `narrowing` collapse the unselected sentinel and the "All"
convention in one place. `Select.BLANK` resolves to the inherited `Widget.BLANK`,
the bool False, so every `is Select.BLANK` guard was dead and every
`not in ("All", Select.BLANK)` was true, letting a NoSelection reach a polars
comparison. `Select.NULL` is the sentinel, and it is reachable from one method.
A load renders once. Select option-setting and value assignment post `Select.Changed`
asynchronously, so a flag cleared when `apply` returns is already clear when those
messages dispatch and each triggers a further filter pass — a full recomputation per
filter in trade_value. The base compares live filter state against what backs the
current render and dispatches only on a real change.
`run_off_thread` owns the worker, the cancellation guard and error routing, so
injury_impact's player search shares the load worker's failure handling.
A long load shows what it is downloading and elapsed time, offers Cancel and binds
escape, and a cancelled load does not render. No percentage is displayed: nflverse
reports no progress, so one would be invented.
─── Data layer ───
loader — one `_cached` helper backs all seven loaders, replacing six copies of the
same cache-check-fetch-store body. Each download runs on its own daemon thread, so
a stalled fetch no longer holds the single shared worker while the next call's
timeout budget runs down against a queue it never left. The weekly-stats fallback
narrows its catch, so a failure that is not data unavailability propagates rather
than silently deriving the season from play-by-play.
cache — parquet and metadata both land through a temporary file and os.replace, so
a crash or a second writer leaves one whole file or the other. A size ceiling
evicts in least-recently-accessed order and orphans are reaped in both directions;
one parquet per season tuple had otherwise accrued without bound against
play-by-play measured in hundreds of megabytes.
seasons — `current_season` and `recent_seasons` resolve the nflverse season label
from an injectable date, replacing thirteen hardcoded season literals across the
six views that needed editing each September.
─── Gates ───
.github/workflows/ci.yml runs lint, format, typecheck, tests with coverage, build,
and a dependency vulnerability scan that fails the build on findings. Installs come
from the committed lockfile, so lockfile drift fails before any gate runs.
pyproject sets mypy `strict = true` with no carve-out, adds ten optional error
codes, and enables `ignore-without-code` and `warn_unused_ignores` so silencing a
check is itself a failure. Coverage measures src/ffb with branch coverage against a
floor taken from measurement.
tests/test_project_gates.py pins the gates against silent relaxation: any boolean
under [tool.mypy] set false fails, since a false there reads as strict while
exempting the check it names.
Views, the base class, both data-layer modules and the app shell gain suites,
driven headlessly through Textual's pilot with the loaders stubbed. The refresh
contract is parametrised across all six views so no tool can regress alone.
Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
…hape Three documented behaviours did not match the code, and one named file did not exist. A claim a reader can act on and be wrong about is worse than no claim. ─── Corrections ─── README described the trade-value production component as season PPG times games remaining, weighted 0.40, alongside a separate 0.10 bye-week penalty. It documents what the engine computes: production is PPG times per-player games remaining, which subtracts a bye still ahead, weighted 0.50, with no standalone bye term. README's injury-impact bullets keyed the confidence tier to a generic sample size and named the wrong season window. Both are read from the engine and the view. CLAUDE.md listed `nfl.import_player_stats`, which the installed nfl_data_py does not export. The importer list names the seven the loaders call, each of which resolves. CLAUDE.md pointed at `nfl-data-api-research.md` for API research and decision rationale. No such file is in the repository. The supplementary-API content it promised already sits inline in CLAUDE.md, so the dangling pointer is removed rather than satisfied with invented research. ─── Additions ─── README gains Configuration, Troubleshooting and Development sections. The first run of any tool is a multi-minute download of two seasons of play-by-play, so Troubleshooting names the elapsed-time readout, the Cancel button and escape binding, the per-download timeout and what its failure means. Configuration states the cache location, its TTL and its size ceiling, each read from the source. docs/architecture.md explains the shape a contributor needs and the code cannot state for itself: why each tool is a bounded context split into a pure engine and a Textual view, why engines take frames and return frames with no I/O, what ToolView owns against what a view owns, how the cache and loader sit underneath, and what adding a seventh tool involves. Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A quality audit graded this repo across ten dimensions and found five correctness defects, a
structural duplication that had already produced a shipped bug, and no CI. This branch closes all
of it. Writing the test suite the audit asked for then surfaced five more defects, which are fixed
here too — the first real test suite finding real bugs is the point of having one.
What changed
Three commits, one per concern.
f06a680— engine correctness. One defect class ran through the engines: a player's displayname standing in where a stable id was needed. Names collide on NFL rosters and vary in spelling
across nflverse source columns, so grouping and joining on them produced plausible-looking wrong
numbers.
snap_share,waiver_wirewindowed by name — two players sharing one merged into a singlerolling series. A steady 90% snap share read as a 36.7% rolling average.
red_zonejoined its receiving and rushing halves on the display name, splitting one GSIS idunder two spellings into two rows — a dual-threat back counted at half his work.
data/statsmerged its four stat lines on the display name, emitting one partial row perspelling.
injury_impactpicked a team-season by iteration order over.unique(), which polars does notguarantee; identical input reported a different season run to run.
start_sit,trade_valuereturned a schema-less empty frame from their guard paths, so thefirst filter raised
ColumnNotFoundError.trade_valueproduction multiplied PPG by a games-remaining term constant across players, so itcancelled in the normalisation that follows and could not affect any output. It is per player and
bye-aware; the standalone bye component folds into it. Weights change to production 0.50,
schedule 0.20, usage 0.20, health 0.10 — rankings move.
trade_valuehealth scaled its denominator with how many seasons carried a report, so identicalabsences scored differently.
data/statscounted fumbles after filtering to pass/run, missing return fumbles, and omitted theposition column entirely on the rosterless path the loader reaches by design.
1585e64— shared lifecycle, data layer, gates. Six views each carried the same loadlifecycle;
_on_data_errorwas byte-identical in all six. Three of the six Refresh buttons reachedthe fetch with
force_refresh=False, re-reading cache while reporting success.ToolViewowns thelifecycle so that divergence is unrepresentable, plus empty-state rendering, one filter-value
reader, and cancel/elapsed-time on long loads. Note
Select.BLANKresolves to the inheritedWidget.BLANK— the boolFalse— so every guard using it was dead. Data layer gets atomic cachewrites with LRU eviction, per-download threads so a stall stops blocking later calls, and
date-injectable season resolution. CI runs lint, format, typecheck, coverage and a build-failing
vulnerability scan; mypy is
strict = truewith no carve-out.10375af— docs. README documented a trade-value formula the engine does not implement andCLAUDE.md named an importer that does not exist and a research file not in the repo. Adds
Configuration, Troubleshooting, Development, and
docs/architecture.md.How tested
ruff format --check .ruff check .mypy src(strict = true, no carve-out)pytest --covuv buildTests go from 13 to 423. One
# type: ignoreinsrc/, on the untypednfl_data_pyimport, withits error code. No test skipped or xfailed; the coverage gate was proven to fail when raised above
the measured total.
Every correctness fix was confirmed by running code, not by reading it — two same-named players
holding separate windows, guard-path frames filtering without raising, per-player production
varying with an upcoming bye,
player_infostable across ten shuffled runs, one id under twospellings yielding one red-zone row, identical absences scoring identical health, and the refresh
contract parametrised across all six views.
Reviewer notes
game out of the remaining slate rather than a flat 10 points off a 0–100 score, so mid-season
byes are materially less punitive and the effect shrinks as the season runs out.
restore,
uv python installon ubuntu-latest and advisory-database reachability are unprovenuntil the first push.
injury_impact/screen.pyat 57%,trade_value74%,start_sit75%. Engines and the data layer are well above the floor.posteam, so a returnfumble is charged to the kicking team's row;
loadercatchesOSErrorbroadly enough that aconnection reset also triggers the play-by-play fallback; and
_team_bye_weeksinfers a bye fromthe first unscheduled week, so a partially loaded schedule would corrupt the production term that
now depends on it.