cal/paper-dynasty-card-creation
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix: remove import-time derived globals in retrosheet_data.py (#14) #48

Merged
cal merged 1 commits from ai/paper-dynasty-card-creation#14 into main 2026-03-23 12:38:01 +00:00
Conversation 0 Commits 1 Files Changed 1 +5 -7
Claude commented 2026-03-21 18:04:22 +00:00
Collaborator
Copy Link

Closes #14

Summary

  • MIN_PA_VL, MIN_PA_VR, MIN_TBF_VL, MIN_TBF_VR, and CARDSET_ID were derived from PLAYER_DESCRIPTION at module import time, creating a hidden ordering dependency: changing PLAYER_DESCRIPTION after import wouldn't update these derived values
  • The CLI (pd_cards/commands/retrosheet.py:142-156) explicitly overrides all of them anyway, so the import-time derivation provided no benefit while creating silent coupling
  • Replaced with plain scalar defaults matching what the original expressions evaluated to (PLAYER_DESCRIPTION = "Live" → MIN_PA_VL=20, MIN_PA_VR=40, CARDSET_ID=27)
  • Also collapsed the dead LAST_WEEK_RATIO = 0.0 if PLAYER_DESCRIPTION == "Live" else 0.0 ternary (both branches were 0.0)

Files changed

  • retrosheet_data.py: lines 56–62 (5 globals), line 70 (dead ternary)

Test results

No test suite. Changes verified by reading the modified file and confirming the diff contains only the targeted lines.

Closes #14 ## Summary - `MIN_PA_VL`, `MIN_PA_VR`, `MIN_TBF_VL`, `MIN_TBF_VR`, and `CARDSET_ID` were derived from `PLAYER_DESCRIPTION` at module import time, creating a hidden ordering dependency: changing `PLAYER_DESCRIPTION` after import wouldn't update these derived values - The CLI (`pd_cards/commands/retrosheet.py:142-156`) explicitly overrides all of them anyway, so the import-time derivation provided no benefit while creating silent coupling - Replaced with plain scalar defaults matching what the original expressions evaluated to (`PLAYER_DESCRIPTION = "Live"` → `MIN_PA_VL=20`, `MIN_PA_VR=40`, `CARDSET_ID=27`) - Also collapsed the dead `LAST_WEEK_RATIO = 0.0 if PLAYER_DESCRIPTION == "Live" else 0.0` ternary (both branches were `0.0`) ## Files changed - `retrosheet_data.py`: lines 56–62 (5 globals), line 70 (dead ternary) ## Test results No test suite. Changes verified by reading the modified file and confirming the diff contains only the targeted lines.
Claude added 1 commit 2026-03-21 18:04:24 +00:00 
Closes #14

Five globals (MIN_PA_VL, MIN_PA_VR, MIN_TBF_VL, MIN_TBF_VR, CARDSET_ID)
were derived from PLAYER_DESCRIPTION at module load time, creating a
hidden ordering dependency: any value baked in before the CLI overrides
PLAYER_DESCRIPTION would be silently wrong if a caller relied on the
derived relationship. The CLI explicitly sets all of them anyway, so
replacing with scalar defaults makes the module self-contained and safe.

Also collapses LAST_WEEK_RATIO dead ternary (both branches were 0.0).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-21 18:15:48 +00:00
Claude reviewed 2026-03-21 18:17:02 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • retrosheet_data.py (modified — lines 56–62, 70)

Findings

Correctness

  • Default values are correct: with PLAYER_DESCRIPTION = "Live", all five conditional expressions evaluate to exactly the new plain scalars (MIN_PA_VL=20, MIN_PA_VR=40, MIN_TBF_VL=20, MIN_TBF_VR=40, CARDSET_ID=27). No behaviour change.
  • The LAST_WEEK_RATIO ternary collapse is correct — both branches were 0.0.
  • Confirmed via pd_cards/commands/retrosheet.py:141-156: the CLI explicitly overwrites all six globals after import, validating the PR's claim that the import-time derivation was dead coupling.

Security

  • No issues found.

Style & Conventions

  • Retained comments (# 1 for PotM, # 27: 2005 Live, 28: 2005 Promos) still document intent for direct-script users — good.
  • Clean, minimal diff. No over-engineering.

Suggestions

  • Overlap with PR #45: PR #45 fixes exactly the same LAST_WEEK_RATIO line (70). Both PRs are open against the same unmerged file. If #45 merges first, #48 will apply the LAST_WEEK_RATIO change redundantly (likely cleanly, but worth being aware of ordering). Consider merging these together or closing #45 in favour of this broader PR.

Verdict: COMMENT (LGTM)

Correct, clean cleanup. The CLI verification confirms the premise. The only observation is the overlap with PR #45 on line 70 — no action required, just merge-order awareness.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `retrosheet_data.py` (modified — lines 56–62, 70) ### Findings #### Correctness - Default values are correct: with `PLAYER_DESCRIPTION = "Live"`, all five conditional expressions evaluate to exactly the new plain scalars (`MIN_PA_VL=20`, `MIN_PA_VR=40`, `MIN_TBF_VL=20`, `MIN_TBF_VR=40`, `CARDSET_ID=27`). No behaviour change. - The `LAST_WEEK_RATIO` ternary collapse is correct — both branches were `0.0`. - Confirmed via `pd_cards/commands/retrosheet.py:141-156`: the CLI explicitly overwrites all six globals after import, validating the PR's claim that the import-time derivation was dead coupling. #### Security - No issues found. #### Style & Conventions - Retained comments (`# 1 for PotM`, `# 27: 2005 Live, 28: 2005 Promos`) still document intent for direct-script users — good. - Clean, minimal diff. No over-engineering. #### Suggestions - **Overlap with PR #45**: PR #45 fixes exactly the same `LAST_WEEK_RATIO` line (70). Both PRs are open against the same unmerged file. If #45 merges first, #48 will apply the `LAST_WEEK_RATIO` change redundantly (likely cleanly, but worth being aware of ordering). Consider merging these together or closing #45 in favour of this broader PR. ### Verdict: COMMENT (LGTM) Correct, clean cleanup. The CLI verification confirms the premise. The only observation is the overlap with PR #45 on line 70 — no action required, just merge-order awareness. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-21 18:17:36 +00:00
cal approved these changes 2026-03-23 12:07:43 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • retrosheet_data.py (modified)

Findings

Correctness

No issues found. The scalar values assigned are correct:

  • MIN_PA_VL = 20 and MIN_PA_VR = 40 match what 20 if "live" in PLAYER_DESCRIPTION.lower() else 1 evaluated to with PLAYER_DESCRIPTION = "Live" (the default).
  • MIN_TBF_VL = 20 and MIN_TBF_VR = 40 are now explicit instead of aliasing MIN_PA_VL/MIN_PA_VR — values are consistent with the CLAUDE.md documentation which lists "Live Series: 20 PA vs L / 40 PA vs R (batters), 20 TBF vs L / 40 TBF vs R (pitchers)".
  • CARDSET_ID = 27 matches the "2005 Live" default.
  • LAST_WEEK_RATIO = 0.0 is correct — both branches of the removed ternary evaluated to 0.0.
  • The CLI override in pd_cards/commands/retrosheet.py (lines 142–156) does explicitly overwrite all five of these globals before calling rd.main([]), confirming the PR's claim that the import-time derivation served no purpose.

Security

No issues found.

Style & Conventions

No issues found. The comments (# 1 for PotM, # 27: 2005 Live, 28: 2005 Promos) are preserved, keeping the intent clear for operators who configure the file directly. The change is conservative and well-scoped.

Suggestions

  • The PLAYER_DESCRIPTION variable at line 42 now has no derived dependents in this file. Its only remaining role is being overridden by the CLI and potentially used elsewhere in retrosheet_data.py. No action required, but worth knowing it is now truly just a documentation signal for direct-run users.

Verdict: APPROVED

The change is correct, minimal, and achieves its stated goal. The removed expressions were genuinely dead weight — the CLI overwrites all five globals unconditionally on every invocation, and the dead 0.0 if ... else 0.0 ternary was pure noise. The PR description accurately describes the diff. No TODOs, FIXMEs, stub code, or security concerns. Safe to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `retrosheet_data.py` (modified) ### Findings #### Correctness No issues found. The scalar values assigned are correct: - `MIN_PA_VL = 20` and `MIN_PA_VR = 40` match what `20 if "live" in PLAYER_DESCRIPTION.lower() else 1` evaluated to with `PLAYER_DESCRIPTION = "Live"` (the default). - `MIN_TBF_VL = 20` and `MIN_TBF_VR = 40` are now explicit instead of aliasing `MIN_PA_VL`/`MIN_PA_VR` — values are consistent with the CLAUDE.md documentation which lists "Live Series: 20 PA vs L / 40 PA vs R (batters), 20 TBF vs L / 40 TBF vs R (pitchers)". - `CARDSET_ID = 27` matches the "2005 Live" default. - `LAST_WEEK_RATIO = 0.0` is correct — both branches of the removed ternary evaluated to `0.0`. - The CLI override in `pd_cards/commands/retrosheet.py` (lines 142–156) does explicitly overwrite all five of these globals before calling `rd.main([])`, confirming the PR's claim that the import-time derivation served no purpose. #### Security No issues found. #### Style & Conventions No issues found. The comments (`# 1 for PotM`, `# 27: 2005 Live, 28: 2005 Promos`) are preserved, keeping the intent clear for operators who configure the file directly. The change is conservative and well-scoped. #### Suggestions - The `PLAYER_DESCRIPTION` variable at line 42 now has no derived dependents in this file. Its only remaining role is being overridden by the CLI and potentially used elsewhere in `retrosheet_data.py`. No action required, but worth knowing it is now truly just a documentation signal for direct-run users. ### Verdict: APPROVED The change is correct, minimal, and achieves its stated goal. The removed expressions were genuinely dead weight — the CLI overwrites all five globals unconditionally on every invocation, and the dead `0.0 if ... else 0.0` ternary was pure noise. The PR description accurately describes the diff. No TODOs, FIXMEs, stub code, or security concerns. Safe to merge. --- *Automated review by Claude PR Reviewer*
cal approved these changes 2026-03-23 12:37:40 +00:00
cal left a comment
Owner
Copy Link

Approved. Removing import-time derived globals eliminates the hidden ordering dependency. Plain scalar defaults are correct given the CLI overrides them explicitly anyway.

Approved. Removing import-time derived globals eliminates the hidden ordering dependency. Plain scalar defaults are correct given the CLI overrides them explicitly anyway.
cal force-pushed ai/paper-dynasty-card-creation#14 from e61ef955f2 to d43927258a 2026-03-23 12:37:55 +00:00 Compare
cal merged commit 770f296938 into main 2026-03-23 12:38:01 +00:00
cal deleted branch ai/paper-dynasty-card-creation#14 2026-03-23 12:38:01 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
cal
Labels
Clear labels   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working
No Label
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-card-creation#48
No description provided.
Delete Branch "ai/paper-dynasty-card-creation#14"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?