Skip to content

Drop the en-passant TODO and lock in current behavior with a regression test - #5

Merged
BrandonZacharie merged 1 commit into
mainfrom
claude/review-chess-py-wktEg
May 24, 2026
Merged

Drop the en-passant TODO and lock in current behavior with a regression test#5
BrandonZacharie merged 1 commit into
mainfrom
claude/review-chess-py-wktEg

Conversation

@BrandonZacharie

Copy link
Copy Markdown
Owner

Summary

Smaller than I pitched. When I dug into the # TODO: Append annotation for En passant take. marker at game/game.py:438, the work was already effectively done — I'd misread the code. The engine's regular pawn-capture branch fires for en passant because Move.perform() sets taken_piece from taken_cell (the captured pawn's actual square, not the empty destination), so the elog already records exd6 — the standard PGN notation for an en passant capture, no e.p. suffix needed. JSON and PGN save/load both round-trip the move correctly.

So instead of changing behavior, this PR:

  1. Replaces the misleading TODO with a comment explaining what the engine does (and why the regular branch handles it).
  2. Adds a regression test that exercises the full en passant path end-to-end: elog token, board state (d5 cleared, d6 holds the white pawn), JSON round-trip, and PGN round-trip via PGNFile. That fence prevents future refactors of the move/capture branches from silently breaking en passant.

Why the previous test sweep didn't catch this implicitly

The Carlsen/Nakamura PGN tests pass moves through PGNFile.game() which uses parse_pgn_move() and then game.move() — the en passant detection in Board.move_piece does the right thing for any pawn diagonal-into-empty move, so the existing tests would have flagged a regression in that path. But the fast-suite tests didn't directly exercise en passant against the engine's own move-side flow, so a refactor that broke the elog token (without breaking parse) could have slipped through. The new test plugs that gap.

What I considered and didn't do

  • Adding e.p. suffix to the elog — would diverge from Carlsen.pgn / Nakamura.pgn conventions and break the equality assertion in test_load_pgn_file. Standard PGN doesn't require it.
  • Adding an ilog event entry (e.g., (d5, "ep")) for cli display — would break the JSON Game.load path, which expects ilog event strings to be piece-promotion letters in PIECE_NAME_TYPE_MAP. Could be done with a load() change but the value/risk ratio is poor.

Test plan

  • make test — fast suite (208 passed, 2 deselected) in ~1.4 s.
  • coverage report after make test shows TOTAL 100%.
  • Manual: play 1.e4 a6 2.e5 d5, capture en passant with e5 → d6. Save as PGN. Confirm movetext ends ...exd6. Reload — board state matches.

https://claude.ai/code/session_01RnQvxtXfn9JFTN8jpNyriL


Generated by Claude Code

Replace the `# TODO: Append annotation for En passant take.` marker
with a comment explaining what the engine already does. On
investigation the work was effectively already done: the regular
pawn-capture branch fires for en passant too because Move.perform()
sets ``taken_piece`` from ``taken_cell`` (the captured pawn's actual
square, not the empty destination), so the elog records 'exd6' which
is the standard PGN notation for an en-passant capture. JSON
save/load and PGN save/load both round-trip the move correctly.

Adds a regression test exercising the full path:
- elog token is 'exd6'
- d5 is cleared, d6 holds the (white) pawn
- JSON save/load preserves elog and board state
- PGN save/load via PGNFile preserves elog and board state

That replaces the implicit "it works if you don't touch it" with an
explicit fence so future refactors of the move/capture branches
don't silently regress en-passant.
@BrandonZacharie
BrandonZacharie merged commit 6696683 into main May 24, 2026
3 checks passed
@BrandonZacharie
BrandonZacharie deleted the claude/review-chess-py-wktEg branch May 24, 2026 06:02
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