Skip to content

Type safety and cleanup refactor#226

Draft
ATGillespie25 wants to merge 2 commits intomainfrom
type-safety-and-cleanup
Draft

Type safety and cleanup refactor#226
ATGillespie25 wants to merge 2 commits intomainfrom
type-safety-and-cleanup

Conversation

@ATGillespie25
Copy link
Copy Markdown
Contributor

Summary

No-behavior-change refactor improving type safety, removing dead code, and reducing duplication across server infrastructure.

Type safety:

  • GameStatus(str, Enum) replacing raw status strings — assignment sites updated, comparisons still work via str inheritance
  • Suit/SpecialRank IntEnums replacing bare int constants, with backward-compatible aliases
  • DiceSet.kept/locked changed from list[int] to set[int] — matches the membership-only semantics, deduplication is automatic
  • RefreshTokenRecord dataclass replacing untyped sqlite3.Row access
  • Database._get_conn() raises RuntimeError instead of letting AttributeError propagate on null connection
  • GameProtocol(Protocol) documenting the 17-mixin contract on Game
  • is_bot property added to User ABC (default False), removing defensive getattr/hasattr checks
  • Table._manager/_server/_db typed under TYPE_CHECKING instead of Any
  • validate_actions() checks handler strings resolve at game start, logs warnings for typos
  • Username reverse index on WebSocketServer for O(1) send_to_user/get_client_by_username

Cleanup:

  • Deleted dead files: main.py (divergent unused entry point), protocol.py (unused packet types), ui/menu.py (unused Menu class)
  • Removed dead tomli fallback imports (project requires Python 3.13+)
  • Removed empty TYPE_CHECKING guards, fixed stale tick-rate comments
  • Extracted _iter_approved_users, _build_action_menu_items, _show_user_list_menu helpers
  • Consolidated config loading through existing load_full_config()
  • Simplified VirtualBotConfig construction with dataclasses.fields() introspection

Tests (60 new):

  • test_cards.py, test_dice.py, test_teams.py, test_round_timer.py, test_game_communication_mixin.py
  • Existing dice-related tests updated for set-typed fields

Test plan

  • All 341 affected tests pass
  • CLI simulate pig --bots 3 completes
  • CLI simulate threes --bots 3 --test-serialization completes (exercises DiceSet set serialization)
  • Reviewed by code-reviewer and silent-failure-hunter agents; critical findings (username index race condition, threes list-to-set assignment, config error handling) fixed before commit

Type safety:
- Add GameStatus(str, Enum) for game lifecycle status
- Add Suit/SpecialRank IntEnums for card constants
- Change DiceSet kept/locked from list[int] to set[int]
- Add RefreshTokenRecord dataclass replacing raw sqlite3.Row
- Add Database._get_conn() null-safe connection helper
- Add GameProtocol documenting the mixin chain contract
- Add is_bot property to User ABC with default False
- Type Table runtime references (Any -> typed under TYPE_CHECKING)
- Add validate_actions() to catch handler string typos at startup
- Add username reverse index for O(1) send_to_user/get_client_by_username

Cleanup:
- Delete dead code: main.py, protocol.py, menu.py
- Remove dead tomli fallback imports (Python 3.13+)
- Remove empty TYPE_CHECKING guards
- Fix stale tick-rate comments
- Extract _iter_approved_users, _build_action_menu_items,
  _show_user_list_menu helpers
- Consolidate config loading via load_full_config()
- Simplify VirtualBotConfig construction with field introspection

Tests:
- Add test_cards, test_dice, test_teams, test_round_timer,
  test_game_communication_mixin (60 new tests)
- Update existing tests for set-typed DiceSet fields
@ATGillespie25 ATGillespie25 marked this pull request as draft April 11, 2026 02:00
server/main.py was removed in the previous commit under the assumption
that __main__.py was the canonical entry point, but the console script
defined in pyproject.toml does not actually work due to the package
layout (server/ contains both the code and pyproject.toml), and
run_server.bat / scripts/run_server.sh both call 'python main.py'.
main.py also does sys.path manipulation that the scripts rely on.
@zarvox32
Copy link
Copy Markdown
Contributor

Tested locally and it's in good shape — 149 tests in the new/updated test files pass, 299 broader tests pass, and CLI simulations complete for pig, threes, yahtzee, twentyone, and milebymile (all with --test-serialization).

A few minor follow-up suggestions, none blocking:

1. Post-rebase sweep for GameStatus (most important)
Since this branched from 0d954fc, main has since merged #215, #225, #231, which touched server/games/milebymile/game.py and server/games/twentyone/game.py and added new self.status = "playing" / "finished" assignments that didn't get converted to the enum. GitHub marks the branch mergeable and the raw-string comparisons still work due to str inheritance, but a quick follow-up pass after rebase would keep the codebase consistent with the PR's stated goal.

2. status field type drift
status: str = GameStatus.WAITING declares str but assigns an enum. After a mashumaro round-trip the field becomes a plain string, so isinstance(status, GameStatus) or type(status) is str checks would behave differently before vs. after save/load. Equality comparisons are unaffected and nothing in the tree relies on this today — just a trap worth flagging for future readers.

3. _get_conn() now raises RuntimeError
Behaviour change from AttributeError to RuntimeError is cleaner, but worth calling out in case anyone had catch blocks tuned to the old type. Grepped — nothing relies on it.

4. validate_actions() logs warnings only
Handler-string typos now surface as WARN on the playpalace.actions logger instead of blowing up at runtime. Good for development ergonomics; worth making sure production logging is configured so a typo doesn't go silent until something explicitly triggers it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants