py-stockfish / stockfish

Integrates the Stockfish chess engine with Python (Official fork)
https://py-stockfish.github.io/stockfish/
MIT License
30 stars 9 forks source link

Refactor and add `num_nodes` and `verbose` options to `get_top_moves()` #16

Closed knutole closed 1 year ago

knutole commented 1 year ago

This PR combines and replaces https://github.com/py-stockfish/stockfish/pull/14 and https://github.com/py-stockfish/stockfish/pull/15 , with major refactor of get_top_moves() function. (It was much easier to combine the PR's, for obvious reasons.) I have left in quite a few comments in the refactor, to make it easier to follow. I will remove most of them once you've had a chance to look it over.

Todo: Add turn_perspective option for rest of codebase, see https://github.com/py-stockfish/stockfish/issues/18

github-actions[bot] commented 1 year ago

Coverage report

The coverage rate went from 93.35% to 94.78% :arrow_up: The branch rate is 88%.

96.72% of new lines are covered.

Diff Coverage details (click to unfold) ### stockfish/models.py `96.72%` of new lines are covered (`94.78%` of the complete file). Missing lines: `660`, `664`
kieferro commented 1 year ago

Wow, that's quite a PR. First of all, a big thank you for the work and the interest that you put in this project! I am currently not able to test and review your code, but it looks like you're still at work anyway. So just ping us or request if you need a review.

knutole commented 1 year ago

Thank you 🙏 This PR is now ready for review 👌

knutole commented 1 year ago

It's not possible for me to request review on this PR, by the way, not sure if there's a Github setting for that. @kieferro @johndoknjas

kieferro commented 1 year ago

It's not possible for me to request review on this PR, by the way, not sure if there's a Github setting for that. @kieferro @johndoknjas

Ah okay, I see. But it seems that nothing can be done about that as long as you don't have write-access. But feel free to just use the draft PR feature to indicate when the PR is ready for review.

johndoknjas commented 1 year ago

Looks like we're all satisfied with the PR, so I've merged it into master.

knutole commented 1 year ago

Great, thanks 🙏👍

kieferro commented 1 year ago

Looks like we're all satisfied with the PR, so I've merged it into master.

Thanks :+1:

Congratulations @knutole. Could you close your two PRs in the old repo (https://github.com/zhelyabuzhsky/stockfish/pull/115, https://github.com/zhelyabuzhsky/stockfish/pull/116)? This would give us a better overview.