fix: replace bare except: with typed NoResultFound in gameplay_queries (#25) #61

Closed
cal wants to merge 13 commits from ai/paper-dynasty-discord-25 into main
Owner

Summary

Replaces all 10 bare except: clauses in in_game/gameplay_queries.py with except sqlalchemy.exc.NoResultFound:.

  • Added import sqlalchemy to imports (consistent with cogs/gameplay.py which uses the same pattern)
  • Replaced bare except: at original lines 127, 238, 310, 333, 356, 447, 470, 493, 702, 811

What was fixed

Bare except: clauses catch all exceptions including KeyboardInterrupt, SystemExit, and GeneratorExit — not just the expected "no result" case. All affected blocks surround .one() calls which raise sqlalchemy.exc.NoResultFound when no row is found. Using a typed exception ensures unrelated errors propagate correctly.

Files changed

  • in_game/gameplay_queries.py

Test results

No test suite (per CLAUDE.md). Changes verified by code review.

Other observations

The ruff linter auto-formatted the file when the edit was saved (reformatted long import lines, standardized quote style, etc.). The cosmetic diff is larger than the functional change as a result, but no logic was altered beyond the import and except clause replacements.

## Summary Replaces all 10 bare `except:` clauses in `in_game/gameplay_queries.py` with `except sqlalchemy.exc.NoResultFound:`. - Added `import sqlalchemy` to imports (consistent with `cogs/gameplay.py` which uses the same pattern) - Replaced bare `except:` at original lines 127, 238, 310, 333, 356, 447, 470, 493, 702, 811 ## What was fixed Bare `except:` clauses catch **all** exceptions including `KeyboardInterrupt`, `SystemExit`, and `GeneratorExit` — not just the expected "no result" case. All affected blocks surround `.one()` calls which raise `sqlalchemy.exc.NoResultFound` when no row is found. Using a typed exception ensures unrelated errors propagate correctly. ## Files changed - `in_game/gameplay_queries.py` ## Test results No test suite (per CLAUDE.md). Changes verified by code review. ## Other observations The ruff linter auto-formatted the file when the edit was saved (reformatted long import lines, standardized quote style, etc.). The cosmetic diff is larger than the functional change as a result, but no logic was altered beyond the `import` and `except` clause replacements.
cal added 1 commit 2026-03-05 12:03:45 +00:00
fix: replace bare except: with typed NoResultFound in gameplay_queries (#25)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m17s
2fac9f38dc
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
ai-reviewing
label 2026-03-05 12:15:32 +00:00
cal reviewed 2026-03-05 12:17:35 +00:00
cal left a comment
Author
Owner

AI Code Review

Files Reviewed

  • in_game/gameplay_queries.py (modified)

Findings

Correctness

  • All 10 bare except: clauses are replaced with except sqlalchemy.exc.NoResultFound:. Count verified against the PR description.
  • sqlalchemy.exc.NoResultFound is the correct exception — it is raised by session.exec(...).one() when no row matches.
  • import sqlalchemy is added at the top of the file, consistent with the existing pattern in cogs/gameplay.py:13.
  • One nuance worth noting: the try blocks contain more than just the .one() call — they also include the update path (setattr, session.add, session.commit, session.refresh). Previously, a session.commit() failure during an update would be silently swallowed by the bare except: and fall through to the insert path, masking the real error. With the typed exception, those commit failures will now correctly propagate up. This is strictly better behavior, though the PR description undersells it by framing the blocks as just "surrounding .one() calls."

Security

  • No issues found.

Style & Conventions

  • Ruff auto-reformatted the file on save (multi-line imports, double quotes, trailing whitespace). No logic was changed by the formatter. The cosmetic diff is benign.
  • Import order: import sqlalchemy is placed before import pydantic in the new code. Ruff accepted this, so no issue.

Suggestions

  • None that would block merging.

Verdict: APPROVED

Clean fix for a real bug class. The exception narrowing is correct and the behavioral change (surfacing previously-swallowed update-path errors) is an improvement. Cosmetic ruff changes are harmless. Ready to merge.


Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `in_game/gameplay_queries.py` (modified) ### Findings #### Correctness - All 10 bare `except:` clauses are replaced with `except sqlalchemy.exc.NoResultFound:`. Count verified against the PR description. - `sqlalchemy.exc.NoResultFound` is the correct exception — it is raised by `session.exec(...).one()` when no row matches. - `import sqlalchemy` is added at the top of the file, consistent with the existing pattern in `cogs/gameplay.py:13`. - **One nuance worth noting:** the try blocks contain more than just the `.one()` call — they also include the update path (`setattr`, `session.add`, `session.commit`, `session.refresh`). Previously, a `session.commit()` failure during an update would be silently swallowed by the bare `except:` and fall through to the insert path, masking the real error. With the typed exception, those commit failures will now correctly propagate up. This is strictly better behavior, though the PR description undersells it by framing the blocks as just "surrounding `.one()` calls." #### Security - No issues found. #### Style & Conventions - Ruff auto-reformatted the file on save (multi-line imports, double quotes, trailing whitespace). No logic was changed by the formatter. The cosmetic diff is benign. - Import order: `import sqlalchemy` is placed before `import pydantic` in the new code. Ruff accepted this, so no issue. #### Suggestions - None that would block merging. ### Verdict: APPROVED Clean fix for a real bug class. The exception narrowing is correct and the behavioral change (surfacing previously-swallowed update-path errors) is an improvement. Cosmetic ruff changes are harmless. Ready to merge. --- *Automated review by Claude PR Reviewer*
cal added
ai-reviewed
and removed
ai-reviewing
labels 2026-03-05 12:18:03 +00:00
cal changed target branch from main to next-release 2026-03-07 07:32:38 +00:00
cal force-pushed ai/paper-dynasty-discord-25 from 2fac9f38dc to c721d0a942 2026-03-07 07:33:36 +00:00 Compare
cal changed target branch from next-release to main 2026-03-23 04:14:23 +00:00
Author
Owner

Closing — will re-implement fresh against main. Original PR had unrebaseable conflicts from the scouting refactor.

Closing — will re-implement fresh against `main`. Original PR had unrebaseable conflicts from the scouting refactor.
cal closed this pull request 2026-03-23 04:15:29 +00:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
1 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#61
No description provided.