cal/paper-dynasty-discord
1
0
Fork 0
Code Issues 14 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: pack rarity targeting, StratGame methods, HR detection #106

Merged
Claude merged 4 commits from fix/gameplay-fixes-batch into main 2026-03-23 04:40:12 +00:00
Conversation 1 Commits 4 Files Changed 3 +2450 -1631
cal commented 2026-03-23 04:31:35 +00:00
Owner
Copy Link

Fixes #20, Fixes #21, Fixes #22

Summary

  • Fix 1 (pack rarity targeting): Replace max_rarity=1 with exact rarity= targeting in the reserve/replacement balancing loop. Previously the else branch used max_rarity=1 which matched both Reserve (1) and Replacement (0) cards. Now uses ('rarity', 0) for Replacement and ('rarity', 1) for Reserve. Applied identically to cogs/economy.py and cogs/economy_new/team_setup.py.

  • Fix 2 (StratGame helper methods): Added get_away_team() and get_home_team() async methods to the StratGame dataclass in db_calls_gameplay.py, each delegating to the existing get_game_team() function with team_id=self.away_team_id / team_id=self.home_team_id. Removed the stale TODO comment from the Game model.

  • Fix 3 (HR detection): In complete_play(), set this_play.batter_final = batter_to_base when batter_to_base is not None before the HR check, then only test this_play.batter_final == 4. Removes the redundant or batter_to_base == 4 path and the # patch to handle little league home runs TODO: comment.

Files Changed

  • cogs/economy.py
  • cogs/economy_new/team_setup.py
  • db_calls_gameplay.py

Test Results

12/12 tests in tests/test_evolution_notifications.py pass. Other test collection errors are pre-existing import issues unrelated to these changes.

Other observations

The pre-commit hook reformatted cogs/economy.py, cogs/economy_new/team_setup.py, and db_calls_gameplay.py (quote style, line wrapping). These cosmetic changes came from the linter running on save — committed with --no-verify per task instructions since the violations are pre-existing. Worth a separate cleanup pass if desired.

🤖 Generated with Claude Code

Fixes #20, Fixes #21, Fixes #22 ## Summary - **Fix 1 (pack rarity targeting):** Replace `max_rarity=1` with exact `rarity=` targeting in the reserve/replacement balancing loop. Previously the `else` branch used `max_rarity=1` which matched both Reserve (1) and Replacement (0) cards. Now uses `('rarity', 0)` for Replacement and `('rarity', 1)` for Reserve. Applied identically to `cogs/economy.py` and `cogs/economy_new/team_setup.py`. - **Fix 2 (StratGame helper methods):** Added `get_away_team()` and `get_home_team()` async methods to the `StratGame` dataclass in `db_calls_gameplay.py`, each delegating to the existing `get_game_team()` function with `team_id=self.away_team_id` / `team_id=self.home_team_id`. Removed the stale TODO comment from the `Game` model. - **Fix 3 (HR detection):** In `complete_play()`, set `this_play.batter_final = batter_to_base` when `batter_to_base is not None` before the HR check, then only test `this_play.batter_final == 4`. Removes the redundant `or batter_to_base == 4` path and the `# patch to handle little league home runs TODO:` comment. ## Files Changed - `cogs/economy.py` - `cogs/economy_new/team_setup.py` - `db_calls_gameplay.py` ## Test Results 12/12 tests in `tests/test_evolution_notifications.py` pass. Other test collection errors are pre-existing import issues unrelated to these changes. ## Other observations The pre-commit hook reformatted `cogs/economy.py`, `cogs/economy_new/team_setup.py`, and `db_calls_gameplay.py` (quote style, line wrapping). These cosmetic changes came from the linter running on save — committed with `--no-verify` per task instructions since the violations are pre-existing. Worth a separate cleanup pass if desired. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 1 commit 2026-03-23 04:31:36 +00:00
fix: pack rarity targeting, StratGame methods, HR detection (#20 #21 #22)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m19s
Details
c0af0c3d32 
- Fix pack distribution to use exact rarity targeting (rarity=0 for
  Replacement, rarity=1 for Reserve) instead of max_rarity=1 which
  matched both tiers; applied to cogs/economy.py and
  cogs/economy_new/team_setup.py

- Add get_away_team() and get_home_team() async methods to StratGame
  dataclass, delegating to get_game_team() with the appropriate
  team_id; remove stale TODO comment from Game model

- Standardize home-run detection in complete_play(): set
  batter_final = batter_to_base when not None before the HR check,
  then only check batter_final == 4 (removes redundant batter_to_base
  path and the patch comment)

Closes #20, Closes #21, Closes #22

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal commented 2026-03-23 04:36:23 +00:00
Author
Owner
Copy Link

Review: REQUEST_CHANGES

Fix 2 (StratGame methods) and Fix 3 (HR detection) are correct.

Fix 1 (pack rarity) has a blocking bug: The players/random API endpoint only accepts min_rarity and max_rarity — not rarity. FastAPI silently ignores unknown params, so ?rarity=0 is discarded and rarity is completely unfiltered. This is worse than the original bug.

Correct fix:

# For Replacement (rarity value 0)
rarity_param = [("min_rarity", 0), ("max_rarity", 0)]
# For Reserve (rarity value 1)
rarity_param = [("min_rarity", 1), ("max_rarity", 1)]

Needs fix in both cogs/economy.py and cogs/economy_new/team_setup.py.

**Review: REQUEST_CHANGES** Fix 2 (StratGame methods) and Fix 3 (HR detection) are correct. **Fix 1 (pack rarity) has a blocking bug:** The `players/random` API endpoint only accepts `min_rarity` and `max_rarity` — not `rarity`. FastAPI silently ignores unknown params, so `?rarity=0` is discarded and rarity is completely unfiltered. This is worse than the original bug. **Correct fix:** ```python # For Replacement (rarity value 0) rarity_param = [("min_rarity", 0), ("max_rarity", 0)] # For Reserve (rarity value 1) rarity_param = [("min_rarity", 1), ("max_rarity", 1)] ``` Needs fix in both `cogs/economy.py` and `cogs/economy_new/team_setup.py`.
cal added 2 commits 2026-03-23 04:37:16 +00:00 
Closes #33
Closes #34

- Delete top-level helpers.py (2153 lines of dead code shadowed by helpers/ package)
- Delete top-level discord_utils.py (251 lines shadowed by helpers/discord_utils.py)
- Fix helpers/main.py: change bare `from discord_utils import *` to relative
  `from .discord_utils import *` so the package import resolves correctly

Note: helpers/main.py has pre-existing ruff violations unrelated to this fix.
--no-verify used to bypass hook for the pre-existing lint debt.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix: use min_rarity/max_rarity for exact rarity targeting
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m27s
Details
55a3255b35 
The players/random API endpoint only accepts min_rarity and max_rarity,
not rarity. The previous fix silently did nothing because FastAPI ignores
unknown query parameters.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal reviewed 2026-03-23 04:39:35 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review (Re-review after rarity fix)

Files Reviewed

  • cogs/economy.py (modified)
  • cogs/economy_new/team_setup.py (modified)
  • db_calls_gameplay.py (modified)

Findings

Fix 1: Pack Rarity Targeting (Previously Blocking)

Verified correct in both files.

The previous blocking issue used ('rarity', N) which the API silently ignored. The fix replaces this with [("min_rarity", N), ("max_rarity", N)] assigned to rarity_params, then unpacked via *rarity_params into the params list.

The * unpacking inside a list literal is valid Python 3.5+ syntax. Confirmed correct expansion:

# Produces: [('pos_include', 'SP'), ('min_rarity', 0), ('max_rarity', 0), ('limit', 2)]
params=[("pos_include", pos), *rarity_params, ("limit", 2)]

The conditional logic is correct:

  • Replacement < Reserve (too few Replacement cards) -> rarity_params = [("min_rarity", 0), ("max_rarity", 0)]
  • Otherwise -> rarity_params = [("min_rarity", 1), ("max_rarity", 1)]

Both the infielder loop and outfielder loop are fixed identically in both cogs/economy.py and cogs/economy_new/team_setup.py. The fix is symmetric and complete.

Fix 2: StratGame Helper Methods

Verified correct.

get_away_team() and get_home_team() are added as async methods on the StratGame dataclass (lines 210-214 in db_calls_gameplay.py). Both delegate to get_game_team(self, team_id=...) using the correct kwarg. The get_game_team function signature accepts team_id as an optional parameter and handles it correctly in both is_pd and non-is_pd branches.

Fix 3: HR Detection

Verified correct.

In complete_play() (lines 1457-1460), the fix assigns this_play.batter_final = batter_to_base when batter_to_base is not None, then checks this_play.batter_final == 4. This correctly eliminates the redundant or batter_to_base == 4 dual-path check and removes the stale # patch to handle little league home runs TODO: comment.

Correctness

  • All three fixes are logically sound and correctly implemented.
  • *rarity_params unpacking expands exactly as expected into the params list.
  • get_game_team kwarg delegation is correct.
  • HR detection now uses a single authoritative field path.

Security

  • No issues found.

Style & Conventions

  • Code follows existing project patterns. The rarity_params variable name clearly communicates intent.
  • Pre-commit cosmetic reformatting (quote style, line wrapping) is noted but pre-existing.

Suggestions

  • The SP/RP pitcher draws at lines 1475/1479 in economy.py still use ("max_rarity", 1) alone without a min_rarity counterpart. Depending on API behavior, this may allow rarity-0 cards to be returned. Not in scope for this PR, but worth a follow-up issue.

Verdict: APPROVED

All three fixes are correctly implemented. The rarity targeting bug is resolved — pinned min_rarity/max_rarity pairs with valid *rarity_params list unpacking replace the previously non-functional ('rarity', N) tuple. The StratGame helper methods and HR detection fix are also intact and correct. This PR is ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review (Re-review after rarity fix) ### Files Reviewed - `cogs/economy.py` (modified) - `cogs/economy_new/team_setup.py` (modified) - `db_calls_gameplay.py` (modified) --- ### Findings #### Fix 1: Pack Rarity Targeting (Previously Blocking) **Verified correct in both files.** The previous blocking issue used `('rarity', N)` which the API silently ignored. The fix replaces this with `[("min_rarity", N), ("max_rarity", N)]` assigned to `rarity_params`, then unpacked via `*rarity_params` into the params list. The `*` unpacking inside a list literal is valid Python 3.5+ syntax. Confirmed correct expansion: ```python # Produces: [('pos_include', 'SP'), ('min_rarity', 0), ('max_rarity', 0), ('limit', 2)] params=[("pos_include", pos), *rarity_params, ("limit", 2)] ``` The conditional logic is correct: - `Replacement < Reserve` (too few Replacement cards) -> `rarity_params = [("min_rarity", 0), ("max_rarity", 0)]` - Otherwise -> `rarity_params = [("min_rarity", 1), ("max_rarity", 1)]` Both the infielder loop and outfielder loop are fixed identically in both `cogs/economy.py` and `cogs/economy_new/team_setup.py`. The fix is symmetric and complete. #### Fix 2: StratGame Helper Methods **Verified correct.** `get_away_team()` and `get_home_team()` are added as `async` methods on the `StratGame` dataclass (lines 210-214 in `db_calls_gameplay.py`). Both delegate to `get_game_team(self, team_id=...)` using the correct kwarg. The `get_game_team` function signature accepts `team_id` as an optional parameter and handles it correctly in both `is_pd` and non-`is_pd` branches. #### Fix 3: HR Detection **Verified correct.** In `complete_play()` (lines 1457-1460), the fix assigns `this_play.batter_final = batter_to_base` when `batter_to_base is not None`, then checks `this_play.batter_final == 4`. This correctly eliminates the redundant `or batter_to_base == 4` dual-path check and removes the stale `# patch to handle little league home runs TODO:` comment. #### Correctness - All three fixes are logically sound and correctly implemented. - `*rarity_params` unpacking expands exactly as expected into the params list. - `get_game_team` kwarg delegation is correct. - HR detection now uses a single authoritative field path. #### Security - No issues found. #### Style & Conventions - Code follows existing project patterns. The `rarity_params` variable name clearly communicates intent. - Pre-commit cosmetic reformatting (quote style, line wrapping) is noted but pre-existing. #### Suggestions - The `SP`/`RP` pitcher draws at lines 1475/1479 in `economy.py` still use `("max_rarity", 1)` alone without a `min_rarity` counterpart. Depending on API behavior, this may allow rarity-0 cards to be returned. Not in scope for this PR, but worth a follow-up issue. --- ### Verdict: APPROVED All three fixes are correctly implemented. The rarity targeting bug is resolved — pinned `min_rarity`/`max_rarity` pairs with valid `*rarity_params` list unpacking replace the previously non-functional `('rarity', N)` tuple. The `StratGame` helper methods and HR detection fix are also intact and correct. This PR is ready to merge. --- *Automated review by Claude PR Reviewer*
Claude added 1 commit 2026-03-23 04:40:06 +00:00
Merge branch 'main' into fix/gameplay-fixes-batch
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m23s
Details
1d7ffb61cd
Claude merged commit 9d1a46b84d into main 2026-03-23 04:40:12 +00:00
Claude deleted branch fix/gameplay-fixes-batch 2026-03-23 04:40:14 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
ai-working

   
bug

   
enhancement

   
feature

New feature proposal

  
in-queue

Approved for development

  
performance

   
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
ai-working
bug
enhancement
feature
in-queue
performance
security
tech-debt
todo
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-discord#106
No description provided.
Delete Branch "fix/gameplay-fixes-batch"

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?