Feat/brute force solution #5
Conversation
📝 WalkthroughWalkthroughThe PR restructures a chess SAN string generation project by migrating root-level Python scripts into a proper package under Changes
Sequence Diagram(s)sequenceDiagram
participant main as __main__
participant gen_sans as gen_sans()
participant iter_space as iter_action_space
participant per_move as gen_sans_for_move()
participant out as write_output_files()
participant files as Output Files
main->>out: write_output_files()
out->>gen_sans: gen_sans()
gen_sans->>iter_space: iterate all moves
iter_space-->>gen_sans: move
gen_sans->>per_move: gen_sans_for_move(move)
per_move->>per_move: classify move type<br/>(pawn/king/nbrq)
per_move->>per_move: generate base SAN
per_move->>per_move: add disambiguators<br/>(file/rank/full)
per_move->>per_move: add check/mate suffixes
per_move-->>gen_sans: set of SAN strings
gen_sans-->>out: aggregated SANs
out->>out: sort by length<br/>then lexicographically
out->>files: san_strings.txt<br/>(base SANs only)
out->>files: san_strings_with_symbols.txt<br/>(with check/mate)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jacksonthall22
force-pushed
the
feat/brute-force-solution
branch
from
e5c3ec3 to
572b2cb
Compare
jacksonthall22
force-pushed
the
feat/brute-force-solution
branch
from
dd66638 to
8df4dd7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
19-24: Update README installation and function references—gen_san_strings.py no longer exists and referenced functions are not in the codebase.Lines 19-24: The quickstart instructs
pip install -r requirements.txtandpython gen_san_strings.py, but:
gen_san_strings.pydoes not exist; the script is nowsrc/san_strings/gen_spoken_san_strings.pyrequirements.txtis outdated (lists onlychess==1.9.1whilepyproject.tomlrequireschess>=1.11.2andchess_action_space>=1.1.0)- Installation should use
pip install .(oruv syncif using uv)Lines 55-58: References to
get_pawn_sans()andget_king_sans()functions ingen_san_strings.pyare incorrect—these functions do not exist in the codebase. Thegen_spoken_san_strings.pyscript is procedural, not function-based.Update both sections to reflect the current package structure: use
pyproject.tomlfor installation, reference the correct script location and name, and remove/correct the function references.
🧹 Nitpick comments (2)
.gitmodules (1)
1-3: Verify GitHub repository accessibility and dependency configuration consistency.The submodule configuration is syntactically correct. However, ensure that:
- The repository at
https://github.com/hyprchs/chess_action_space.gitis accessible (public or properly authenticated)- The dependency in
pyproject.tomlcorrectly references this submodule (e.g., with a path dependency or proper version constraint)- Developer setup documentation exists for initializing the submodule after cloning
If this is a new submodule, consider adding a note in your project's setup documentation instructing developers to run
git clone --recurse-submodulesorgit submodule update --init --recursiveafter pulling.src/san_strings/gen_spoken_san_strings.py (1)
9-9: Drop the unused enumerate index.Since the loop index isn’t used, you can drop
enumerate(or rename the binding to_) to satisfy Ruff’s B007 warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.gitignore(1 hunks).gitmodules(1 hunks)README.md(1 hunks)gen_san_strings.py(0 hunks)gen_spoken_san_strings.py(0 hunks)pyproject.toml(1 hunks)src/chess_action_space(1 hunks)src/san_strings/__init__.py(1 hunks)src/san_strings/constants.py(1 hunks)src/san_strings/gen_spoken_san_strings.py(1 hunks)src/san_strings/san_strings_with_symbols.txt(0 hunks)src/san_strings/utils/__init__.py(1 hunks)src/san_strings/utils/san_parts.py(1 hunks)
💤 Files with no reviewable changes (3)
- gen_spoken_san_strings.py
- src/san_strings/san_strings_with_symbols.txt
- gen_san_strings.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/san_strings/utils/san_parts.py (1)
src/san_strings/utils/__init__.py (6)
attacked_by_mask(37-80)extend_ray(158-178)is_diagonal_slider(16-17)is_orthogonal_slider(20-21)is_slider(24-25)ordinal(28-34)
src/san_strings/__init__.py (2)
src/san_strings/utils/__init__.py (6)
can_be_king_move(328-360)can_be_nbrq_move_by(281-302)can_be_pawn_move(228-278)can_require_file_disambiguator(363-385)can_require_full_square_disambiguator(413-434)can_require_rank_disambiguator(388-410)src/san_strings/utils/san_parts.py (20)
KingSanParts(862-972)NBRQSanParts(417-859)PawnSanParts(210-414)SanParts(28-207)copy(206-207)check_or_mate(71-72)check_or_mate(75-78)rendered(82-82)rendered(259-265)rendered(454-462)rendered(878-884)can_cause_check(168-176)can_cause_checkmate(179-185)to_square(67-68)file_disambiguator(251-252)file_disambiguator(446-447)is_capture(63-64)promotion_piece_type(255-256)piece_type(59-60)rank_disambiguator(450-451)
🪛 Ruff (0.14.4)
src/san_strings/gen_spoken_san_strings.py
9-9: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
src/san_strings/utils/san_parts.py
35-37: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
150-152: Avoid specifying long messages outside the exception class
(TRY003)
160-162: Avoid specifying long messages outside the exception class
(TRY003)
201-201: Avoid specifying long messages outside the exception class
(TRY003)
222-224: Avoid specifying long messages outside the exception class
(TRY003)
232-234: Avoid specifying long messages outside the exception class
(TRY003)
238-240: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Unused method argument: place_kings
(ARG002)
274-276: Avoid specifying long messages outside the exception class
(TRY003)
304-304: Avoid specifying long messages outside the exception class
(TRY003)
356-360: Avoid specifying long messages outside the exception class
(TRY003)
389-389: Avoid specifying long messages outside the exception class
(TRY003)
407-407: Avoid specifying long messages outside the exception class
(TRY003)
429-431: Avoid specifying long messages outside the exception class
(TRY003)
433-433: Avoid specifying long messages outside the exception class
(TRY003)
435-435: Avoid specifying long messages outside the exception class
(TRY003)
468-468: Unused method argument: place_kings
(ARG002)
471-473: Avoid specifying long messages outside the exception class
(TRY003)
499-504: Avoid specifying long messages outside the exception class
(TRY003)
518-522: Avoid specifying long messages outside the exception class
(TRY003)
561-565: Avoid specifying long messages outside the exception class
(TRY003)
584-588: Avoid specifying long messages outside the exception class
(TRY003)
622-626: Avoid specifying long messages outside the exception class
(TRY003)
650-655: Avoid specifying long messages outside the exception class
(TRY003)
691-695: Avoid specifying long messages outside the exception class
(TRY003)
719-723: Avoid specifying long messages outside the exception class
(TRY003)
727-729: Avoid specifying long messages outside the exception class
(TRY003)
825-825: Avoid specifying long messages outside the exception class
(TRY003)
847-847: Avoid specifying long messages outside the exception class
(TRY003)
849-851: Avoid specifying long messages outside the exception class
(TRY003)
890-890: Unused method argument: place_kings
(ARG002)
893-895: Avoid specifying long messages outside the exception class
(TRY003)
920-924: Avoid specifying long messages outside the exception class
(TRY003)
949-949: Avoid specifying long messages outside the exception class
(TRY003)
954-954: Avoid specifying long messages outside the exception class
(TRY003)
963-963: Avoid specifying long messages outside the exception class
(TRY003)
965-967: Avoid specifying long messages outside the exception class
(TRY003)
src/san_strings/__init__.py
39-39: Avoid specifying long messages outside the exception class
(TRY003)
src/san_strings/utils/__init__.py
206-206: Avoid specifying long messages outside the exception class
(TRY003)
265-265: Avoid specifying long messages outside the exception class
(TRY003)
287-290: Avoid specifying long messages outside the exception class
(TRY003)
302-302: Avoid specifying long messages outside the exception class
(TRY003)
356-358: Avoid specifying long messages outside the exception class
(TRY003)
369-369: Avoid specifying long messages outside the exception class
(TRY003)
371-371: Avoid specifying long messages outside the exception class
(TRY003)
394-394: Avoid specifying long messages outside the exception class
(TRY003)
396-396: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
.gitignore (1)
3-4: Thanks for ignoring local environment artifacts.Adding
.idea/and.venv/keeps editor config and virtualenv clutter out of version control.pyproject.toml (1)
1-40: Configuration looks consistent.Good to see the project metadata, dependency pinning, and tool configs spelled out together; the local path override for
chess_action_spaceshould keep the submodule and package in sync.src/san_strings/constants.py (1)
1-52: Constants and regexes look solid.Nice to see the python-chess masks reused here; the pawn and non-pawn SAN patterns cover the expected disambiguators and suffixes cleanly.
| @@ -0,0 +1 @@ | |||
| Subproject commit 2631675d3a1c8bdae9781d520ac7d922d080abbc | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify submodule integration and document initialization requirements.
Adding a git submodule introduces coordination challenges that need proper documentation and dependency configuration. For projects with subpackages such as those with git submodules, documentation in a README file or tool configuration is required to clarify the steps needed.
🏁 Script executed:
#!/bin/bash
# Verify submodule configuration and documentation
set -e
echo "=== Checking .gitmodules ==="
if [ -f .gitmodules ]; then
echo "✓ .gitmodules exists"
cat .gitmodules
else
echo "✗ .gitmodules not found"
exit 1
fi
echo ""
echo "=== Checking pyproject.toml dependency declaration ==="
if grep -q "chess_action_space" pyproject.toml 2>/dev/null; then
echo "✓ chess_action_space found in pyproject.toml"
rg -A 2 "chess_action_space" pyproject.toml
else
echo "✗ chess_action_space not declared in pyproject.toml"
fi
echo ""
echo "=== Checking README for submodule initialization docs ==="
if [ -f README.md ]; then
if grep -qi "git clone.*recursive\|submodule\|chess_action_space" README.md; then
echo "✓ README mentions submodule/initialization"
rg -i "git clone.*recursive|submodule|chess_action_space" README.md
else
echo "⚠ README may not document submodule initialization"
fi
else
echo "⚠ README.md not found"
fi
echo ""
echo "=== Checking for setup scripts (setup.py or pre-install hooks) ==="
if [ -f setup.py ]; then
echo "setup.py exists - checking for submodule initialization"
rg -i "submodule|git.*init" setup.py
fi
if [ -f pyproject.toml ]; then
if grep -q "\[build-system\]" pyproject.toml; then
echo "pyproject.toml has build-system config"
fi
fiLength of output: 621
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full context on dependencies and check for additional docs
set -e
echo "=== Full pyproject.toml dependencies context ==="
rg -B 3 -A 5 'chess_action_space' pyproject.toml
echo ""
echo "=== Checking for development/installation documentation ==="
for file in CONTRIBUTING.md INSTALL.md docs/INSTALL.md docs/setup.md docs/development.md docs/README.md; do
if [ -f "$file" ]; then
echo "Found: $file"
if grep -qi "submodule\|git clone.*recursive\|chess_action_space" "$file"; then
echo " ✓ Contains submodule/setup guidance"
rg -i "submodule|git clone.*recursive|chess_action_space" "$file"
else
echo " ✗ Does not mention submodule setup"
fi
fi
done
echo ""
echo "=== Checking pyproject.toml build-system backend ==="
grep -A 3 "\[build-system\]" pyproject.toml || echo "No [build-system] found"
Length of output: 547
---
🏁 Script executed:
```shell
#!/bin/bash
# Final check: verify README exists and get its content to assess where/how to add docs
if [ -f README.md ]; then
echo "=== README.md exists - checking length and structure ==="
wc -l README.md
echo ""
echo "=== First 50 lines of README.md ==="
head -50 README.md
echo ""
echo "=== Checking for Installation/Setup section ==="
grep -n "## Installation\|## Setup\|## Getting Started\|## Development" README.md || echo "No standard setup section found"
else
echo "README.md not found"
fi
Length of output: 3103
Update README installation instructions to document submodule initialization requirement.
The README's "Run it yourself" section now has outdated installation instructions that don't account for the git submodule dependency. Developers cloning without --recursive will get an empty src/chess_action_space/ directory and encounter broken imports or build failures without clear guidance.
Update the installation section to document:
- Use
git clone --recursive(or rungit submodule update --init --recursiveafter cloning) - That
chess_action_spaceis a required submodule dependency
🤖 Prompt for AI Agents
In src/chess_action_space around lines 1 to 1, the README installation
instructions are missing mention of the required git submodule; update the "Run
it yourself" section to instruct users to clone the repo with git clone
--recursive or run git submodule update --init --recursive after cloning, and
explicitly state that src/chess_action_space is a required submodule dependency
so developers know to initialize/update submodules before building.
| file = san_string[idx + 1] | ||
| rank = san_string[idx + 2] | ||
| assert file in 'abcdefgh' | ||
| assert rank in '12345678' | ||
| idx += 3 | ||
|
|
||
| # Ex. 'rook takes' or 'rook takes e 4' or 'rook takes rook' | ||
| words.append( | ||
| ('', f'{file} {rank}', 'pawn', 'knight', 'bishop', 'rook', 'queen') | ||
| ) | ||
| elif c == '=': | ||
| words.append(('', 'equals')) | ||
| # break | ||
| else: | ||
| words.append(c) | ||
|
|
||
| idx += 1 | ||
|
|
There was a problem hiding this comment.
Captures drop everything after the destination square.
Inside the capture branch we do idx += 3, then the loop’s trailing idx += 1 fires. That means any suffix after the destination (promotion =Q, +, #, etc.) is skipped entirely. For example, exd8=Q never sees the = token, so the promotion wording is missing; Qxe4+ drops the check suffix. Please adjust the index math so we only skip the destination squares, not the rest of the SAN string.
Apply this fix:
- idx += 3
+ idx += 2With that change, captures still consume the destination square, and the following iteration will process promotion/check/mate markers correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| file = san_string[idx + 1] | |
| rank = san_string[idx + 2] | |
| assert file in 'abcdefgh' | |
| assert rank in '12345678' | |
| idx += 3 | |
| # Ex. 'rook takes' or 'rook takes e 4' or 'rook takes rook' | |
| words.append( | |
| ('', f'{file} {rank}', 'pawn', 'knight', 'bishop', 'rook', 'queen') | |
| ) | |
| elif c == '=': | |
| words.append(('', 'equals')) | |
| # break | |
| else: | |
| words.append(c) | |
| idx += 1 | |
| file = san_string[idx + 1] | |
| rank = san_string[idx + 2] | |
| assert file in 'abcdefgh' | |
| assert rank in '12345678' | |
| idx += 2 | |
| # Ex. 'rook takes' or 'rook takes e 4' or 'rook takes rook' | |
| words.append( | |
| ('', f'{file} {rank}', 'pawn', 'knight', 'bishop', 'rook', 'queen') | |
| ) | |
| elif c == '=': | |
| words.append(('', 'equals')) | |
| # break | |
| else: | |
| words.append(c) | |
| idx += 1 |
🤖 Prompt for AI Agents
In src/san_strings/gen_spoken_san_strings.py around lines 50 to 67, the capture
branch currently does idx += 3 then the loop's trailing idx += 1 runs, causing
one extra character after the destination square to be skipped (dropping
promotion/check/mate markers). Change the capture branch so it only advances
past the destination square before the loop's trailing increment — replace idx
+= 3 with idx += 2 — ensuring the subsequent iteration will process any '=' '+',
'#' or other suffixes.
| if dr == 2: | ||
| # Must be white pawn move | ||
| if (from_rank, to_rank) != (1, 3): | ||
| return False | ||
| elif dr == -2: | ||
| # Must be black pawn move | ||
| if (from_rank, to_rank) != (6, 4): | ||
| return False |
There was a problem hiding this comment.
Fix double-step pawn checks
Because dr is computed as from_rank - to_rank, white double pushes produce -2 and black double pushes produce +2. The current branches assume the opposite, so both (from_rank, to_rank) == (1, 3) and (6, 4) fail their guards and every two-square pawn advance (e.g., e4, ...e5) gets rejected. That prevents gen_sans_for_move from ever emitting SAN strings for double pawn pushes. Please flip the sign-specific branches so both colors pass their respective validation.
- if dr == 2:
- # Must be white pawn move
- if (from_rank, to_rank) != (1, 3):
+ if dr == -2:
+ # Must be white pawn move
+ if (from_rank, to_rank) != (1, 3):
return False
- elif dr == -2:
- # Must be black pawn move
- if (from_rank, to_rank) != (6, 4):
+ elif dr == 2:
+ # Must be black pawn move
+ if (from_rank, to_rank) != (6, 4):
return False🤖 Prompt for AI Agents
In src/san_strings/utils/__init__.py around lines 256 to 263, the double-step
pawn check uses dr = from_rank - to_rank but treats dr sign backwards; flip the
sign-specific branches so white double pushes use dr == -2 and validate
(from_rank, to_rank) == (1, 3), and black double pushes use dr == 2 and validate
(from_rank, to_rank) == (6, 4); update the comments to match the corrected sign
logic.
Summary by CodeRabbit
New Features
Documentation
Chores