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

Fixed the stockfish_major_version on stockfish development builds #124

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

PanagiotisIatrou
Copy link

@PanagiotisIatrou PanagiotisIatrou commented Dec 17, 2022

There was an error when using stockfish development builds.

stockfish\models.py", line 57, in init
self._stockfish_major_version: int = int(
^^^^
ValueError: invalid literal for int() with base 10: 'dev202212127cf93f8b'

Also fixed some type annotations and reformatted models.py

@johndoknjas
Copy link
Contributor

Hi @PanagiotisIatrou thanks, this looks good. I wasn't aware of that format for the version, so good to know.

For the failing checks, it's just an issue with an existing test function. If you do something similar to this it should help: 56ea969

@PanagiotisIatrou
Copy link
Author

Hello @johndoknjas. It seems that the fen == "8/8/8/3k4/3K4/8/8/8 b - - 0 1" check is failing. Do you have any idea why that would be?

@johndoknjas
Copy link
Contributor

johndoknjas commented Dec 18, 2022

@PanagiotisIatrou For that test it depends on the version of SF that's being used. The function it tests basically works by making a temporary new stockfish executable, feeding it an FEN, and seeing if it crashes (if so, the FEN is invalid). It's pretty hacky but the docs for the function at least mention that it'll give the wrong answer sometimes.

@johndoknjas
Copy link
Contributor

@PanagiotisIatrou Oh I see the checks are still failing. I forgot that for my branch I linked, in a later commit I removed the test case altogether: 200a725 So it should be fine to do this.

I've made an issue for this function and included the fen of the test case there, so in the future maybe it'll be re-added if the function can be improved.

@github-actions
Copy link

Coverage report

The coverage rate went from 86% to 94% ⬆️
The branch rate is 85%.

66% of new lines are covered.

Diff Coverage details (click to unfold)

stockfish/models.py

67% of new lines are covered (94% of the complete file).

Missing lines: 66, 70

@PanagiotisIatrou
Copy link
Author

@johndoknjas thanks for pointing it out. I temporarily removed this check and it successfully runs all the checks now.

@jasiukiewicztymon
Copy link

jasiukiewicztymon commented Feb 28, 2023

Hi, can someone merge it if it works?

@HippoProgrammer
Copy link

This is the issue documented on #135.

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.

4 participants