Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Premove #29

Merged
merged 6 commits into from
May 22, 2024
Merged

Premove #29

merged 6 commits into from
May 22, 2024

Conversation

LKL1235
Copy link
Contributor

@LKL1235 LKL1235 commented May 21, 2024

premove function which #27 discussed

Closes #27

@trevorbayless
Copy link
Owner

trevorbayless commented May 21, 2024

Thanks for the PR! I haven't had time to look at it quite yet, but there are some lint errors that need to be addressed.

You can run flake8 locally to verify you have fixed the errors. To do so, in your venv install cli-chess with dev dependencies by using the following command: pip install -e .[dev]. You should then navigate to cli-chess/src/cli_chess and run flake8.

Note: If you have issues with pip install -e .[dev] then try pip install -e '.[dev]'

Copy link
Owner

@trevorbayless trevorbayless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR! I've added some comments to be addressed.

A couple additions to the review:

  • The "PreMove" text is not appearing on online games.
  • Premoves currently will kill an entire online game session if the pre-move is accepted but is then invalid after the opponent moves. Example: Starting as white, play e4 and premove e5. If the opponent plays e5 the game is then in a bad state. You should be able to replicate this easily in an online game against Stockfish. When quitting cli-chess after this happens, you will see exceptions in the console.

.vscode/launch.json Outdated Show resolved Hide resolved
src/cli_chess/modules/premove/premove_view.py Outdated Show resolved Hide resolved
src/cli_chess/modules/premove/premove_view.py Outdated Show resolved Hide resolved
src/cli_chess/modules/premove/premove_presenter.py Outdated Show resolved Hide resolved
src/cli_chess/modules/premove/premove_presenter.py Outdated Show resolved Hide resolved
src/cli_chess/modules/premove/premove_presenter.py Outdated Show resolved Hide resolved
src/cli_chess/modules/premove/premove_view.py Outdated Show resolved Hide resolved
src/cli_chess/core/game/offline_game/offline_game_model.py Outdated Show resolved Hide resolved
src/cli_chess/core/game/offline_game/offline_game_model.py Outdated Show resolved Hide resolved
@LKL1235
Copy link
Contributor Author

LKL1235 commented May 22, 2024

Thanks again for the PR! I've added some comments to be addressed.

A couple additions to the review:

  • The "PreMove" text is not appearing on online games.
  • Premoves currently will kill an entire online game session if the pre-move is accepted but is then invalid after the opponent moves. Example: Starting as white, play e4 and premove e5. If the opponent plays e5 the game is then in a bad state. You should be able to replicate this easily in an online game against Stockfish. When quitting cli-chess after this happens, you will see exceptions in the console.

Thanks for guide, I did fix them and commit

@trevorbayless trevorbayless merged commit 0d73bf9 into trevorbayless:master May 22, 2024
13 checks passed
@trevorbayless trevorbayless mentioned this pull request May 24, 2024
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.

Premoves
2 participants